-
Notifications
You must be signed in to change notification settings - Fork 6
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
Convert Datetime to Astropy Time #107
Convert Datetime to Astropy Time #107
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #107 +/- ##
==========================================
+ Coverage 94.28% 94.47% +0.19%
==========================================
Files 10 10
Lines 1767 1757 -10
==========================================
- Hits 1666 1660 -6
+ Misses 101 97 -4 ☔ View full report in Codecov by Sentry. |
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.
All of the time object changes look good but this merge request seems to be doing more than just this. Could you further describe what other issues you are solving here? Thanks!
@@ -1321,8 +1299,7 @@ def _get_generation_date(self, data): | |||
""" | |||
Function to get the date that the CDF was generated. | |||
""" | |||
day = datetime.date.today() | |||
return datetime.datetime(day.year, day.month, day.day) | |||
return Time.now().strftime("%Y-%m-%d") |
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.
Should further specify the date here in case we generate two CDF files in a single day?
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.
@ehsteve The previous implementation just included the date (no time) so I followed that paradigm. We would only have naming conflicts if the other portions of the name were also the same: instrument, start time, mode, data level...
I do not think we'll meet these conditions where all aspects are the same, but if you think we will, I can update to include a timestamp in addition to the date.
This PR converts use of the
datetime
library to use exclusivelyastropy.time.Time
objects within the HERMES data container.datetime
objects are still required for interactions with thespacepy
library because spacepy does not have Astropy time support. However, this improves consistency within the data container itself so thedatetime
library is only used in the IO functionality needed to load and save CDFs with spacepy.closes #102