-
Notifications
You must be signed in to change notification settings - Fork 3
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
374 current time settings not working for long duration 600 years #375
374 current time settings not working for long duration 600 years #375
Conversation
failing test are due to a bug in hydropandas that will be solved by: ArtesiaWater/hydropandas#246 |
Tests now fail because of numpy version. I solved this in PR #373 by pinning the numpy version for ci in pyproject.toml. |
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.
Really nice work! I left some comments here and there.
One more general comment, that might be a bit controversial 🙈 as you put in quite some work to get cftime to work automatically:
There seems to be some repetition of cftime conversion in each of the different paths to construct a time index. This functionality is nice, but also adds quite some complexity to set_ds_time()
. Just wondering whether you considered trapping the pandas OutofBounds error, and just returning an error message to the user to convert the input (start or time) to cftime types? And then having just the one path for constructing a time index using cftime?
Just a thought, so feel free to dismiss it since the automatic functionality is already fully operational now :).
Thanks for the review and you are right. I was trapped by the sunk cost fallacy . I will update the PR |
- default to float time index - fix for minor error introduced in #375
No description provided.