DM-23044: PTC task should persist usable linearity models #364
Conversation
| @@ -74,7 +76,7 @@ def check_metadata(obj, valid_start, instrument, chip_id, filepath, data_name): | |||
| """ | |||
| md = obj.getMetadata() | |||
| finst = md['INSTRUME'] | |||
| fchip_id = md['DETECTOR'] | |||
| fchip_id = md['DETECTOR_ID'] | |||
There was a problem hiding this comment.
I don't think you can change this because all the other calibration datasets use DETECTOR. This routine only works if we have standardized metadata. (although I don't know how the defects tests passed with this change). @SimonKrughoff what am I missing?
There was a problem hiding this comment.
I think this definitely can't change. I have no idea how the tests didn't fail. I can look into it.
There was a problem hiding this comment.
I've confirmed on a local checkout that this does break unit tests.
There was a problem hiding this comment.
I'll go back to 'DETECTOR'. This is ambiguous and was stopping the code when I was passing the detector name instead of the id, which is what it actually expects. That's why I thought that changing the name would help clarify.
There was a problem hiding this comment.
Please remember to squash your new change with the previous commit so that the change to the header name disappears from the history.
There was a problem hiding this comment.
Perhaps it deserves a comment in the code, but since I had pointed you at the CLO post in the past, I was surprised you found it ambiguous to the point that you'd change it unilaterally.
There was a problem hiding this comment.
Please forgive me, but I can't find in my records the post that you are talking about. When I was trying to make the code work, I probably did not remember this post that you mention and decided to change the code in order to move forward in a way that I thought would make things clearer. I have changed the keyword back to just DETECTOR, so it's OK. I was also surprised to find that it was not clear by the name of the keyword that it was expecting an ID and not a name: after a few rounds of crashing and debugging and looking at the source code, then I was able to tell.
There was a problem hiding this comment.
Here's an example: https://lsstc.slack.com/archives/C2JPMCF5X/p1575927103181300
You respond by saying
Thank you for pointing this out again! Sorry that I forgot. I’ll ask if I run into problems.
So I think there are other examples.
There was a problem hiding this comment.
Thank you for posting this out again! I'm OK with changing the keyword back to "DETECTOR", which I already did, although I think that perhaps it should be "DETECTORID" or "DETECTORINDEX", IMO.
| factory_map = {'qe_curve': Curve, 'defects': Defects} | ||
| files = glob.glob(os.path.join(root, chip_name, '*.ecsv')) | ||
| factory_map = {'qe_curves': Curve, 'defects': Defects, 'linearizer': Linearizer} | ||
| files = glob.glob(os.path.join(root, chip_name, '*.ecsv')) + glob.glob(os.path.join(root, |
There was a problem hiding this comment.
In the interests of future proofing can we do this as:
files = []
extensions = (".ecsv", ".yaml")
for ext in extensions:
files.extend(glob.glob(os.path.join(root, chip_name, f"*.{ext}")))?
plazas
force-pushed
the
tickets/DM-23044
branch
from
ff93606
to
f6f6690
Compare
| @@ -25,8 +26,12 @@ def read_one_chip(root, chip_name, chip_id): | |||
| A dictionary of objects constructed from the appropriate factory class. | |||
| The key is the validity start time as a `datetime` object. | |||
| """ | |||
| factory_map = {'qe_curve': Curve, 'defects': Defects} | |||
| files = glob.glob(os.path.join(root, chip_name, '*.ecsv')) | |||
| factory_map = {'qe_curves': Curve, 'defects': Defects, 'linearizer': Linearizer} | |||
There was a problem hiding this comment.
You need to turn this back to qe_curve. It's a rebase error (it used to be qe_curves but we changed it to qe_curve and on rebase you reverted that change).
plazas
force-pushed
the
tickets/DM-23044
branch
2 times, most recently
from
1d69c00
to
d8ae82e
Compare
No description provided.