spacetelescope / romancal

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

Put real file names in meta.resample.meta. #1209

Closed schlafly closed 5 months ago

schlafly commented 5 months ago

This PR puts the real file names in meta.resample.members rather than the individual file name meta.filename attributes. This is intended to address https://github.com/spacetelescope/romancal/issues/1154 , but is rather awkward.

The challenge is that the ModelContainer passed to resample doesn't know about the original file names. This PR digs around the private contents of the datamodel to grab the input file name, and if that fails, falls back to img.meta.filename. But relying on img._asdf._fname is clearly asking for trouble.

The alternative would be to teach ModelContainer about the input file names if it's read from an association. That would add a new filenames member to ModelContainer that defaults to None, and sets the new filenames to be equal to _models if everything is a string. Then resample would check that attribute and use it if it's not None.

Thoughts on what the preferred approach is here? @ddavis-stsci , @mairanteodoro , you may have thoughts here?

Checklist

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 79.05%. Comparing base (3d7d787) to head (b61e70b). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1209 +/- ## ========================================== + Coverage 79.03% 79.05% +0.02% ========================================== Files 116 116 Lines 7985 7994 +9 ========================================== + Hits 6311 6320 +9 Misses 1674 1674 ``` | [Flag](https://app.codecov.io/gh/spacetelescope/romancal/pull/1209/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) | Coverage Δ | | *Carryforward flag | |---|---|---|---| | [nightly](https://app.codecov.io/gh/spacetelescope/romancal/pull/1209/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) | `62.78% <ø> (ø)` | | Carriedforward from [3d7d787](https://app.codecov.io/gh/spacetelescope/romancal/commit/3d7d787b43592995d90d5f04918a77c09fae2e80?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) | *This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ddavis-stsci commented 5 months ago

I think that the container should store the asn member names. As is, the container loses the given filename and assumes that the meta.filename is the name on disk. That is clearly incorrect here. If I understand your code you want to reset the meta.filename for the member files and rewrite them to disk? I'm not sure that is a good idea. If I understood Tyler correctly they would be happy with storing the asn member filenames in the output products? I assume you think that this is not sufficient?

schlafly commented 5 months ago

Yes, the alternative I proposed is adding a new attribute to the model container that contains the file names. I can code that option up later.

mairanteodoro commented 5 months ago

I too agree with using ModelContainer to handle the ASN member names.