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

Populate sky metadata with zero if matching is not possible. #1360

Merged
merged 6 commits into from
Aug 13, 2024

Conversation

schlafly
Copy link
Collaborator

@schlafly schlafly commented Aug 9, 2024

Make sure that we send sensible metadata even when the L2 images don't overlap one another. The existing code populates Nones here, which don't fit into the individual_image_metadata tables with the other numbers.

Regression tests are passing: #1355

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.58%. Comparing base (8d7d7aa) to head (dd006ea).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1360   +/-   ##
=======================================
  Coverage   78.57%   78.58%           
=======================================
  Files         117      117           
  Lines        7861     7862    +1     
=======================================
+ Hits         6177     6178    +1     
  Misses       1684     1684           
Flag Coverage Δ *Carryforward flag
nightly 62.26% <ø> (ø) Carriedforward from 8d7d7aa

*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

This fixes the issue I was seeing,

rerun tests
strun --debug roman_mos r0000101001001001001_r274dp63x32y80_visit_F158_prompt_i2d_asn.json
2024-08-12 10:20:41,812 - stpipe.MosaicPipeline - INFO - MosaicPipeline instance created.
...
2024-08-12 10:32:58,174 - stpipe.MosaicPipeline - INFO - Saved model in r0000101001001001001_r274dp63x32y80_visit_F158_prompt_i2d.asdf
[stpipe.MosaicPipeline] Saved model in r0000101001001001001_r274dp63x32y80_visit_F158_prompt_i2d.asdf
2024-08-12 10:32:58,174 - stpipe.MosaicPipeline - INFO - Step MosaicPipeline done
[stpipe.MosaicPipeline] Step MosaicPipeline done

asdfattr.py r0000101001001001001_r274dp63x32y80_visit_F158_prompt_i2d.asdf cal_step
attributes: cal_step
ex_list: ['cal_step']
Meta Attribute cal_step
flux COMPLETE
outlier_detection COMPLETE
skymatch COMPLETE
resample COMPLETE

Do we need this in for the release. My take is that it would be nice but not needed.

@schlafly
Copy link
Collaborator Author

Regression tests are passing. https://github.com/spacetelescope/RegressionTests/actions/runs/10365669711

It doesn't address a specific requirement and is in that sense probably optional, but I don't see why we wouldn't include it. Likewise I think we should get in #1355, as it fixes a bug, but isn't critical. The latter requires some okify'ing, but only to get rid of the previous problematic extra context planes.

@schlafly schlafly marked this pull request as ready for review August 13, 2024 10:02
@schlafly schlafly requested a review from a team as a code owner August 13, 2024 10:02
@schlafly schlafly merged commit 7d6c9d7 into spacetelescope:main Aug 13, 2024
30 checks passed
@schlafly schlafly deleted the always-populate-sky-metadata branch August 13, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants