JP-3482: Load units from FITS default header keywords directly #238
Conversation
tapastro
force-pushed
the
JP-3482-spec-units
branch
from
5b81bc4
to
73c3468
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #238 +/- ##
==========================================
- Coverage 64.71% 64.62% -0.09%
==========================================
Files 102 102
Lines 5668 5685 +17
==========================================
+ Hits 3668 3674 +6
- Misses 2000 2011 +11 ☔ View full report in Codecov by Sentry. |
| @@ -57,3 +61,17 @@ def __init__(self, init=None, **kwargs): | |||
| return | |||
|
|
|||
| super(MultiSpecModel, self).__init__(init=init, **kwargs) | |||
|
|
|||
| try: | |||
| if isinstance(init, fits.hdu.hdulist.HDUList): | |||
There was a problem hiding this comment.
Not sure I like the idea of hardwiring FITS-isms into our datamodels. Kind of breaks the paradigm of making them disk format independent. What happens if someone in the future ever wants to store a MultiSpecModel in an ASDF file, instead of FITS?
There was a problem hiding this comment.
Ideally this would/should be handled at a lower level somewhere down in the actual datamodels code and/or its interface to fitsio.
There was a problem hiding this comment.
I agree in principle, and hopefully @braingram has a better idea of how to accomplish this. But I don't see a method defined that connects a schema-defined table with unit designations, and I'm unsure of how to even start that. Even just saving an existing MultiSpecModel from fits to asdf, then loading the asdf gives different data structure types for the table - the asdf file returns <class 'asdf.tags.core.ndarray.NDArrayType'> with no column attributes, while the fits version returns <class 'astropy.io.fits.fitsrec.FITS_rec'>. I don't know enough about the asdf ndarray to understand if it allows units, but the FITS_rec does allow this, with the downside that it stores that data in the FITS keywords...
There was a problem hiding this comment.
FWIW, @nden pointed me to the unit schemas in asdf that may or may not be used in Roman datamodels, but I don't know if/how they're being implemented for tables.
There was a problem hiding this comment.
Thanks for putting this together. I should hopefully have a chance to look into this PR tomorrow in more depth. One question I have is: does make sense to put the units in the schema? There are a few other schemas that have units (here's one example). I am not sure if or how the units in the schema are used. It might be possible to have stdatamodels use the units in the schema during write to define the TUINT headers and on read to fill in the units from the schema (perhaps instead of or only if there are no TUNIT headers).
There was a problem hiding this comment.
The units here are variable, which makes me think that example would be hard to mimic - I tried to pull the unit schema from asdf into the schema directory here and enter something like:
- name: WAVELENGTH
datatype: float64
unit:
- ref: unit.schema
in the spectable.schema.yaml file, but from what I can tell, datamodels does not have any method of storing/validating attributes of a table/array beyond name and datatype.
And no rush, I know you're out of the office today! Just leaving a tag in your log to look into tomorrow - thanks for taking a look!
There was a problem hiding this comment.
Thanks! I wasn't aware these were variable (and agree this makes adding them to the schema not an option).
It appears that the _cast that occurs during reading a datamodel from a fits file loses the units. I think the changes here should fix this specific issue: master...braingram:stdatamodels:fitsrec_units
I opened an issue to try to organize and document the current state of unit support and hopefully come up with a plan: #240
There was a problem hiding this comment.
Should I close this PR and have a new one for your fixes, or should I just commit those into this PR and remove my edits?
There was a problem hiding this comment.
It may be easiest if I open a new one. I sent the link to @jemorrison and if the new PR doesn't fix the issue (but this one does) then I'll close the new PR.
|
Is this being superseded by #243 and hence can be closed? |
|
This has definitely been superseded and can be closed! |
Resolves JP-3482
Closes [spacetelescope/jwst#8109]
This PR addresses a lack of specified units in the spec/spectable jwst schemas, resulting in metadata not propagating through a save/load cycle. Additionally, loading a spectrum into a datamodel will not have any unit strings accessible. This feels like hacky workaround, but does generate unit strings in the spec_table. Looking forward to expert review 🙂
Checklist
CHANGES.rst(either inBug FixesorChanges to API)