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
DM-43109: Add class method for calculating observing day #72
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #72 +/- ##
==========================================
+ Coverage 79.60% 79.70% +0.09%
==========================================
Files 36 36
Lines 3133 3148 +15
Branches 670 672 +2
==========================================
+ Hits 2494 2509 +15
Misses 508 508
Partials 131 131 ☔ 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.
Looks good, a couple of minor comments.
if isinstance(offset, int): | ||
offset = astropy.time.TimeDelta(offset, format="sec", scale="tai") | ||
observing_date -= offset | ||
return int(observing_date.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.
Is it possible to do it without doing string conversion? This may fail for year 999 if strftime
adds 0 at the start (but maybe not, and you can add base=10
).
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.
A TAI for year less than 1970-ish is not allowed.
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.
It does seem to work fine:
>>> t.Time("0999-02-05T12:45:00", format="isot", scale="tai").strftime("%Y%m%d")
'09990205'
>>> int(t.Time("0999-02-05T12:45:00", format="isot", scale="tai").strftime("%Y%m%d"))
9990205
I haven't worked out how to get the year/month/day without the string formatting. It only takes 18 microseconds to do the string formatting and cast to int though.
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.
My idea was that internally strftime
already does its job to extract year/month/day, so you can save few microseconds doing just arithmetic instead of string conversion, but if it too hard then strings are OK.
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.
Right, if only I knew how to ask for the year, month, and day as integers.
This helps other systems that need to calculate the observing day without having a header to translate.
This helps other systems that need to calculate the observing day without having a header to translate. Is required by lsst-dm/daf_butler_migrate#36.