fix: Replaced assert with ValueError in stochastic_occupancy#109
fix: Replaced assert with ValueError in stochastic_occupancy#109igopalakrishna wants to merge 1 commit intogoogle:copybara_pushfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
igopalakrishna
force-pushed
the
fix-assertions
branch
from
d848cad to
adf06bb
Compare
igopalakrishna
force-pushed
the
fix-assertions
branch
from
adf06bb to
e452979
Compare
igopalakrishna
changed the title
s2t2
left a comment
s2t2
left a comment
There was a problem hiding this comment.
Nice, thanks @igopalakrishna for taking up this issue. 🙌 I provided some specific comments below for the first instance you fixed.
In general, it would be nice to have a single PR that addresses all the instances across the codebase.
With the error raising implemented, we should now be able to add some tests to ensure each of the errors is triggered when we pass in certain invalid conditions. If you are able to add a corresponding test for each error, that would be awesome!
| ): | ||
| raise ValueError( | ||
| "Time bounds must be in chronological order i.e., " | ||
| "expected arrival/departure hours must be strictly increasing:" "earliest_expected_arrival_hour < latest_expected_arrival_hour " |
There was a problem hiding this comment.
I think we want the second string on its own line, to avoid line-too-long errors.
There was a problem hiding this comment.
let's consider using more natural language in the error message, like "earliest expected arrival must come before latest expected arrival, earliest expected departure must come before latest expected departure", etc.
|
@igopalakrishna should we close this PR in favor of #113 ? |
2 participants
Fixes #28
This pull request resolves the issue by replacing
assertstatements with explicitValueErrorexceptions in the example file mentioned,smart_control/simulator/stochastic_occupancy.py.Using
ValueErroris better practice asassertstatements can be disabled in production environments, which would otherwise silently ignore important precondition checks.