-
Notifications
You must be signed in to change notification settings - Fork 164
changing TimeExtrapolationError to OutsideTimeInterval #2329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@erikvansebille |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start to fixing issue #2145! But you should propably also update the import statement on /src/parcels/__init.py__, as the unit tests now fail?
src/parcels/_core/kernel.py
Outdated
|
|
||
| ErrorsToThrow = { | ||
| StatusCode.ErrorTimeExtrapolation: _raise_time_extrapolation_error, | ||
| StatusCode.ErrorOutsideTimeInterval: _raise_time_extrapolation_error, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This _raise_time_extrapolation_error function should probably also be renamed to _raise_outside_time_interval_error?
|
Thanks for the PR here :)
@joseCarlosAndrade have you seen our contributing documentation on the v4-dev branch? (in particular, the Pixi workflows on how to test locally?) If you follow those and those tests pass, then they'll also pass in CI |
for more information, see https://pre-commit.ci
…deTimeInterval error
c133178 to
a5cc9b6
Compare
|
Hey! Thank you all for your help, we really appreciate the tutoring! I've made some updates in this task:
While running the tests, I encountered some warnings and possible errors that I believe may not be related to this scope. The output from running: Is this expected in v4-dev? Could someone help me understand the causes of this output, please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good; I'm happy to merge this PR. Thanks for the contribution @joseCarlosAndrade!
Yes, this is indeed expected output. The |
|
Thanks for the help guys! Great work with this project 👏 |
|
Thanks everyone for the help! |
Changing
TimeExtrapolationErrortoOutsideTimeInterval, as pointed by this issueWe're from the team of @vicenzodarezzo , which flagged the assignment of this issue on the original Parcels repo.