spacetelescope / stdatamodels

https://stdatamodels.readthedocs.io
Other
5 stars 25 forks source link

Several AMI related fits keywords in keyword dictionary not in datamodel schemas #342

Open braingram opened 1 month ago

braingram commented 1 month ago

HDU: PRIMARY KEYWORD: CALIB HDU: PRIMARY KEYWORD: PA

These may have been added in anticipation of future datamodel/pipeline work.

See: https://jira.stsci.edu/browse/JWSTKD-545

rcooper295 commented 4 weeks ago

I populated (or tried to populate?) the CALIB keyword here: https://github.com/spacetelescope/jwst/blob/main/jwst/ami/oifits.py#L583 It will only be in the aminorm-oi.fits products output by the AmiAnalyze step, not the "raw" ami-oi.fits products output by the AmiAnalyze step. There are some details here I might not be clear on; can there be keywords listed in a datamodel that are optional? The 'PA' keyword seems to have been left out, I think that was an oversight on my part, so I can include populating it in the PR I'll be opening soon for some other AMI changes. I guess we'll need a concurrent stdatamodels update as well, if I'm understanding correctly that the issue is the keywords not being included in the schema itself?

braingram commented 4 weeks ago

Thanks!

The datamodel schemas have no definition for CALIB. To have the value appear in the FITS headers an entry will need to be added to the schemas. At the moment the line:

self.calib_oimodel.meta.oifits.calib = calname

doesn't produce a CALIB FITS keyword and instead just adds an attribute to the datamodel tree (which is stored in the ASDF portion of the FITS file).

It is possible to list optional values in the datamodel schema (this is the default and extra schema keywords are needed to make something a requirement).

The jwst files are extra complicated because there is also a keyword dictionary which separately lists all the FITS keywords in a different format. At the moment there are CALIB and PA keywords in the keyword dictionary but not in the datamodel schemas. Since the keywords are not in the schema they will never end up in a file.

rcooper295 commented 4 weeks ago

Thanks for the explanation! So will the line above that puts calname in the datamodel tree as calib work as-is after the CALIB keyword has been added to the schema? I was under the impression that some magic happens under the hood where the metadata and FITS headers kind of cross-populate each other -- i.e. if I update a FITS header keyword in a file, the corresponding metadata keyword would also be updated, and vice versa? If that's not completely untrue, is it only true if the keyword is defined in the datamodel schema (and presumably also the JWST keyword dictionary?)

braingram commented 4 weeks ago

Possibly. It depends on how we define CALIB in the schema. I'm not seeing CALIB mentioned in either oifits paper. Is it an oifits keyword? If not it probably makes sense to put it somewhere else in the datamodel tree and to map that location to CALIB.

The mapping between datamodel tree location and FITS keyword is defined in the datamodel schemas (we can ignore the keyword dictionary as it's unused by cal).

For example consider the beginning of the "core" schema: https://github.com/spacetelescope/stdatamodels/blob/1236c76086eb13a647e54304f66e7685bf9177a1/src/stdatamodels/jwst/datamodels/schemas/core.schema.yaml#L6-L14 which (in part) maps meta.date to the fits keyword DATE (in the default PRIMARY HDU). For CALIB (and possibly PA) we could add those in a similar way to either the amioi schema or if the norm files have more differences perhaps a different schema and datamodel.

rcooper295 commented 3 weeks ago

Sorry, I wrote a reply here but I guess didn't actually hit the comment button, and when my computer restarted it was lost... CALIB isn't an official OIFITS keyword, we added it because there were cases where a single target is calibrated by multiple reference stars, and this was a quicker way of finding out which was which than having to go back and find the reference star OIFITS file. I think the OIFITS standard allows for additional keywords and data columns to be added? I see, so there is an explicit mapping to the FITS keyword. Yes, it might make sense to do that for CALIB and PA. I think that the inclusion of the CALIB keyword and the PISTON_T and PISTON_C data columns in place of the single PISTONS keyword are the only differences between the raw ami-oi files and the aminorm-oi files.

braingram commented 3 weeks ago

Thanks!

Additional keywords and data columns should be fine (they are allowed by the OIFITS standard and the jwst datamodel schemas).

I think for CALIB and PA it makes sense to put them in a different schema (not in the oifits schema directly). Let me know if a more detailed look is helpful (and a PR to update the schemas). If so I can see when I can fit that in.

rcooper295 commented 3 weeks ago

Thanks Brett! If it's not too much trouble for you to assist with a more detailed look at what changes need to be made that would be great. I'm definitely getting more familiar with the datamodels and can take a stab at it myself but that might end up taking longer for both of us. I also wanted to make sure @melanieclarke was in the loop on this as she's been my pipeline point-person for the rest of these changes.

braingram commented 3 weeks ago

Thanks Brett! If it's not too much trouble for you to assist with a more detailed look at what changes need to be made that would be great. I'm definitely getting more familiar with the datamodels and can take a stab at it myself but that might end up taking longer for both of us. I also wanted to make sure @melanieclarke was in the loop on this as she's been my pipeline point-person for the rest of these changes.

Certainly! I'm happy to help. Let me take a look at your draft jwst PR, the keyword dictionary tickets and schemas. I should hopefully be up to speed by mid-week and reach out with questions. Let me know if it's more urgent than that and we can set up a time to chat.

rcooper295 commented 3 weeks ago

Thanks so much! Let me know if there's any info I can help provide to clarify things. Mid-week would be amazing, I have a nominal goal to have the PR moved out of draft stage and towards actually being merged by the end of the week but that was before I knew about the datamodel changes we would need to make, so that can be flexible.

braingram commented 3 weeks ago

How "sold" are you on "meta.oifits.calib" and "meta.oifits.pa" for the datamodel metadata attributes? How about "meta.calibrator_object_id" and "meta.telescope_roll_angle" instead: https://github.com/spacetelescope/stdatamodels/pull/357

rcooper295 commented 3 weeks ago

That looks great to me! I'm not particularly attached to that nomenclature, so I'm happy to adopt your suggested ones.

rcooper295 commented 3 weeks ago

This will probably need to be opened as a separate issue, but while we're talking about datamodels, I have some more questions about FITS keywords. I'm working on changing the AMI code to use values from the header of the NRM reference file rather than the hardcoded ones currently in mask_definitions.py. When I load the reference file as an NRMModel, the keywords I need are in the extra_fits part of the tree, but instead of being accessed like this they seem to be stored in nested lists and have to be accessed by their index:

nrm_model = datamodels.NRMModel(newnrm)
# get value of header keyword 'NRM_X_A1'
nrm_x_a1 = nrm_model.extra_fits.PRIMARY.header[6][1]
# doesn't work:
nrm_x_a1 = nrm_model._extra_fits.PRIMARY.NRM_X_A1

I'm wondering if I'm just opening the datamodel the wrong way, or if it's an issue with how the fits file was written in the first place, or something else? This whole issue may be irrelevant anyways; since these values will be important to the AMI code I was thinking it might make sense to have them be defined/required in the schema.

braingram commented 2 weeks ago

Thanks! Would you open a separate issue? The reference file keywords aren't listed in the keyword dictionary and are in different schemas in this package. Adding them to the schema is required to get them out of "extra_fits". Adding them will require picking an attribute name so feel free to suggest one (or we can just use the current variable names) in that issue.