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

Add only_use_cftime_datetimes flag to num2date #39

Merged
merged 2 commits into from
Apr 25, 2018

Conversation

spencerkclark
Copy link
Collaborator

Closes #37

I tried to go for the path of least resistance. I added a flag to DateFromJulianDay that allows one to bypass the logic that uses datetime.datetime objects in certain situations (and exposed that flag in netcdftime.num2date). If only_use_netcdftime_datetimes is True we have the following behavior:

  • For calendar='standard', DatetimeGregorian objects are always used
  • For calendar='gregorian', DatetimeGregorian objects are always used
  • For calendar='proleptic_gregorian', DatetimeProlepticGregorian objects are always used

If only_use_netcdftime_datetimes is False (which is the default), then the original behavior occurs.

Does this seem like something reasonable? I'm happy to discuss things further.

@@ -669,15 +678,21 @@ def DateFromJulianDay(JD, calendar='standard'):
if calendar in 'proleptic_gregorian':
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In looking at this logic, I noticed that as written this branch of the if-statement is used for both calendar='proleptic_gregorian' and calendar='gregorian'. Therefore, while the next branch is elif calendar in ('standard', 'gregorian') it will never be used in the case of calendar='gregorian'. Was this intended?

In other words, should this line be if calendar == 'proleptic_gregorian'?

@spencerkclark
Copy link
Collaborator Author

Appveyor seems to be failing in setting up the environment for Python 2.7, so I suspect it is unrelated to the changes I've made here (the same thing seems to be happening in #38).

@spencerkclark spencerkclark mentioned this pull request Apr 12, 2018
4 tasks
@spencerkclark spencerkclark changed the title Add only_use_netcdftime_datetimes flag to num2date Add only_use_cftime_datetimes flag to num2date Apr 13, 2018
@spencerkclark
Copy link
Collaborator Author

This is now updated in accordance with the renaming of the package to cftime; the option in num2date is now called only_use_cftime_datetimes.

@jhamman jhamman requested a review from jswhit April 17, 2018 07:22
@jhamman
Copy link
Collaborator

jhamman commented Apr 17, 2018

@spencerkclark - Thanks, this looks good. I'd like @jswhit to have a quick look before merging but generally, its nice to have the added consistency!

@jhamman jhamman merged commit 64f7484 into Unidata:master Apr 25, 2018
@spencerkclark spencerkclark deleted the num2date-netcdftime-datetimes branch April 25, 2018 20:13
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