spacetelescope / romancal

Python library to process science observations from the Nancy Grace Roman Space Telescope
https://roman-pipeline.readthedocs.io/en/latest/
Other
32 stars 28 forks source link

Review group_id use in tweakreg #1450

Open schlafly opened 1 week ago

schlafly commented 1 week ago

Presently tweakreg stores the group_id from the model library in the meta.group_id of each individual output SCA. We don't have that in the model schema. It's unclear to me that we want it, but if we did want it, we should probably store it somewhere intentional, rather than at the root level of the Roman metadata. It seems to me that stcal.tweakreg is responsible for setting this, but probably tweakreg shouldn't be writing special stuff into the root level of the datamodel metadata?

Additionally, we do some tests in tweakreg that seem to assume some structure for the group_ids. https://github.com/spacetelescope/romancal/blob/1945dad5eb600fe82acb865baf3f1d66da12dd7f/romancal/tweakreg/tests/test_tweakreg.py#L912-L927 It's unclear to me where that group_id construction is coming from. Maybe this test just needs to be simplified.

braingram commented 1 week ago

This is coming from the ModelLibrary (which inherited this behavior from ModelContainer): https://github.com/spacetelescope/romancal/blob/1945dad5eb600fe82acb865baf3f1d66da12dd7f/romancal/datamodels/library.py#L62 It sets:

schlafly commented 1 week ago

Thanks Brett. Yes. And it looks like we share this behavior with Webb (i.e., it's not a Roman invention?). https://github.com/spacetelescope/stpipe/blob/main/src/stpipe/library.py#L539-L541 For my two cents this information is probably better kept at the library level, and things like exptype we probably have elsewhere in the schema? But it's not worth reorganizing this now.

braingram commented 1 week ago

Thanks! I think all of these could be removed.

I could see how the library could be updated to get exptype and group_id from other metadata attributes. Since group_id == meta.observation.obs_id that's straightforward. exptype is trickier. I don't see anything equivalent in the schema. Given the limited use maybe there's a way to avoid resolving it for non-library models by saying:

The library in stpipe would need a few more hooks to allow the subclass in romancal to implement this logic (and the jwst subclass updated to allow it to continue to use meta.exptype, etc).