Skip to content

374 current time settings not working for long duration 600 years#375

Merged
OnnoEbbens merged 11 commits intodevfrom
374-current-time-settings-not-working-for-long-duration-600-years
Oct 11, 2024
Merged

374 current time settings not working for long duration 600 years#375
OnnoEbbens merged 11 commits intodevfrom
374-current-time-settings-not-working-for-long-duration-600-years

Conversation

@OnnoEbbens
Copy link
Collaborator

No description provided.

@OnnoEbbens OnnoEbbens linked an issue Oct 7, 2024 that may be closed by this pull request
@OnnoEbbens OnnoEbbens marked this pull request as draft October 7, 2024 16:52
@OnnoEbbens OnnoEbbens marked this pull request as ready for review October 8, 2024 17:03
@OnnoEbbens
Copy link
Collaborator Author

failing test are due to a bug in hydropandas that will be solved by: ArtesiaWater/hydropandas#246

@rubencalje
Copy link
Collaborator

Tests now fail because of numpy version. I solved this in PR #373 by pinning the numpy version for ci in pyproject.toml.

Copy link
Collaborator

@dbrakenhoff dbrakenhoff left a 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 :).

@OnnoEbbens
Copy link
Collaborator Author

Thanks for the review and you are right. I was trapped by the sunk cost fallacy . I will update the PR

@OnnoEbbens OnnoEbbens merged commit d7b6bb8 into dev Oct 11, 2024
dbrakenhoff added a commit that referenced this pull request Oct 14, 2024
- default to float time index
- fix for minor error introduced in #375
@OnnoEbbens OnnoEbbens deleted the 374-current-time-settings-not-working-for-long-duration-600-years branch March 27, 2025 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Current time settings not working for long duration (>600 years)

3 participants