Skip to content
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

use UDUNITS seconds_per_year #333

Conversation

MarcoGorelli
Copy link

@MarcoGorelli MarcoGorelli commented Feb 17, 2023

As discussed on Slack, let's use UDUNITS in seconds_per_year

If users have a calendar other than Gregorian, then conversion needs to happen before they use pyleoclim

@MarcoGorelli MarcoGorelli force-pushed the more-precise-fractional-datetimes branch 2 times, most recently from b81d3b7 to 9a53f61 Compare February 17, 2023 17:32
@MarcoGorelli MarcoGorelli marked this pull request as draft February 17, 2023 18:13
@MarcoGorelli MarcoGorelli force-pushed the more-precise-fractional-datetimes branch from 9a53f61 to 47a8a12 Compare February 20, 2023 10:50
@MarcoGorelli MarcoGorelli changed the title Dont hard-code seconds per year use UDUNITS seconds_per_year Feb 20, 2023
@MarcoGorelli MarcoGorelli force-pushed the more-precise-fractional-datetimes branch from 47a8a12 to 87bf306 Compare February 20, 2023 10:56
@MarcoGorelli MarcoGorelli marked this pull request as ready for review February 20, 2023 10:56
@MarcoGorelli MarcoGorelli force-pushed the more-precise-fractional-datetimes branch from 87bf306 to ae23a77 Compare February 20, 2023 11:12
@CommonClimate
Copy link
Collaborator

Hi @MarcoGorelli , thanks for completing this. In terms of review, it is a little hard to tell what was done with the forced pushed changes. Can you summarize in what way the datetimes are "more precise"? Is it literally because UDUNITS has more decimal points in its definition of the number of days per year than 365.25? I'm asking because it looks like you also made quite a few changes in tsbase.py, and would like to understand what they are doing and why (at a high level).

@MarcoGorelli
Copy link
Author

Hey

originally I was interpreting 20.1 as meaning 20 years plus 0.1 times the number of seconds in the next year, whereas on Slack we discussed that it should mean 20.1 * SECONDS_PER_YEARS. So I amended the logic to do that - it's much simpler like that it was before, which is nice

The other changes are just adding extra tests, docstrings, and amending the existing ones

@CommonClimate
Copy link
Collaborator

OK, I'll merge, then.

@CommonClimate CommonClimate merged commit 18137be into LinkedEarth:Development Feb 20, 2023
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.

2 participants