change internal time system of L1 products to be SCETime or UTC #447
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #447 +/- ##
==========================================
- Coverage 72.23% 67.04% -5.19%
==========================================
Files 78 76 -2
Lines 8085 8136 +51
==========================================
- Hits 5840 5455 -385
- Misses 2245 2681 +436 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
samaloney
left a comment
samaloney
left a comment
There was a problem hiding this comment.
I'm not sure allowing operations in different time systems is a good idea. A product should either have time in OBT or UTC and the conversion should only be done once in the from_level0 or similar methods?
All products (>L0) will always have obt_timerange from products will have an utc_timerange but only if the time sys in header is UTC and not sure supporting on the fly conversion is a good idea because the spice kernel dependence.
This aren't blocker just thoughts
| assert p.exposure == p.data["timedel"].as_float().min().to_value("s") | ||
| assert p.max_exposure == p.data["timedel"].as_float().max().to_value("s") | ||
| else: | ||
| assert p.exposure == p.data["timedel"].min().to_value("s") | ||
| assert p.max_exposure == p.data["timedel"].max().to_value("s") |
There was a problem hiding this comment.
I'd avoid asserts like this for use if and raise a specific exception
There was a problem hiding this comment.
we test if the xxx_exposure property is working for L1 and L0 files in the same test.
both have different internal timing systems: so i need the if case or we split it up into 2 separate tests.
stixcore/products/product.py
Outdated
| if "TIMESYS" in pri_header and pri_header["TIMESYS"] == "UTC": | ||
| try: | ||
| offset = Time(pri_header["DATE-OBS"]) | ||
| except Exception: |
| if "timedel" in l1.data.colnames and isinstance(l1.data["timedel"], SCETimeDelta): | ||
| l1.data.replace_column( | ||
| "timedel", | ||
| l1.data["timedel"].as_float(), |
There was a problem hiding this comment.
Is this an int to start with?
There was a problem hiding this comment.
no here it is a SCETimeDelta and the as_float() does the conversion to seconds
| @@ -524,10 +537,22 @@ def __init__( | |||
|
|
|||
| @property | |||
| def scet_timerange(self): | |||
There was a problem hiding this comment.
For this specific case we always have OBT-BEG and OBT-END so maybe this should use those?
There was a problem hiding this comment.
.obt_beg and end are only existing properties for LB products
There was a problem hiding this comment.
Yes but we always have the headers all L0+ fits files have OBT_BEG and OBT_END
There was a problem hiding this comment.
yes but only if it comes from FITS read or after it was just writen out to fits. Some intermediate states of products will not have valid header data. Frech created with init or splited into days voa to_days ....
stixcore/products/product.py
Outdated
| (self.data["time"][0] - self.data["timedel"][0] / 2).datetime, | ||
| (self.data["time"][-1] + self.data["timedel"][-1] / 2).datetime, |
There was a problem hiding this comment.
Is the .datetime really needed?
|
i agree that it is not ideal to provide some of the on the fly conversions. The auto converting most of the time (and if proper used never) will not happen as our process steps will do this explicit. In general this L1 product could have state depended time systems and i think it would be bad experience if the properties will raise an exception if used |
3 participants
No description provided.