Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

schlafly
Copy link
Collaborator

@schlafly schlafly commented May 1, 2024

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 #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

@schlafly schlafly added this to the 24Q3_B14 milestone May 1, 2024
Copy link

codecov bot commented May 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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 Coverage Δ *Carryforward flag
nightly 62.78% <ø> (ø) Carriedforward from 3d7d787

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ddavis-stsci
Copy link
Collaborator

ddavis-stsci commented May 1, 2024

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
Copy link
Collaborator Author

schlafly commented May 1, 2024

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
Copy link
Collaborator

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

@schlafly schlafly force-pushed the real-filenames-in-resample-members branch from 1d31f61 to b61e70b Compare May 2, 2024 17:03
@schlafly schlafly marked this pull request as ready for review May 2, 2024 18:53
@schlafly schlafly requested a review from a team as a code owner May 2, 2024 18:53
Copy link
Collaborator

@ddavis-stsci ddavis-stsci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@schlafly schlafly merged commit ae63531 into spacetelescope:main May 3, 2024
30 checks passed
@schlafly schlafly deleted the real-filenames-in-resample-members branch May 3, 2024 13:03
@braingram braingram mentioned this pull request May 22, 2024
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants