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

Hackday mteodoro sky variance readnoise #1556

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

mairanteodoro
Copy link
Collaborator

@mairanteodoro mairanteodoro commented Dec 9, 2024

Resolves RCAL-968

This PR adds a sky variance array to the mosaic file created by resample (i.e. model.var_sky). It also enables building a drizzle image weighted by the inverse of the sky variance when running resample with weight_type="ivsky".

Regression test

Tasks

  • request a review from someone specific, to avoid making the maintainers review every PR
  • add a build milestone, i.e. 24Q4_B15 (use the latest build if not sure)
  • Does this PR change user-facing code / API? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • update or add relevant tests
    • update relevant docstrings and / or docs/ page
    • start a regression test and include a link to the running job (click here for instructions)
      • Do truth files need to be updated ("okified")?
        • after the reviewer has approved these changes, run okify_regtests to update the truth files
  • if a JIRA ticket exists, make sure it is resolved properly
news fragment change types...
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change
  • changes/<PR#>.docs.rst
  • changes/<PR#>.stpipe.rst
  • changes/<PR#>.associations.rst
  • changes/<PR#>.scripts.rst
  • changes/<PR#>.mosaic_pipeline.rst
  • changes/<PR#>.patch_match.rst

steps

  • changes/<PR#>.dq_init.rst
  • changes/<PR#>.saturation.rst
  • changes/<PR#>.refpix.rst
  • changes/<PR#>.linearity.rst
  • changes/<PR#>.dark_current.rst
  • changes/<PR#>.jump_detection.rst
  • changes/<PR#>.ramp_fitting.rst
  • changes/<PR#>.assign_wcs.rst
  • changes/<PR#>.flatfield.rst
  • changes/<PR#>.photom.rst
  • changes/<PR#>.flux.rst
  • changes/<PR#>.source_detection.rst
  • changes/<PR#>.tweakreg.rst
  • changes/<PR#>.skymatch.rst
  • changes/<PR#>.outlier_detection.rst
  • changes/<PR#>.resample.rst
  • changes/<PR#>.source_catalog.rst

Copy link

codecov bot commented Dec 9, 2024

Codecov Report

Attention: Patch coverage is 68.18182% with 7 lines in your changes missing coverage. Please review.

Project coverage is 77.50%. Comparing base (f538ba0) to head (af738f3).

Files with missing lines Patch % Lines
romancal/resample/resample_utils.py 61.11% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1556      +/-   ##
==========================================
- Coverage   77.94%   77.50%   -0.44%     
==========================================
  Files         115      115              
  Lines        7622     7643      +21     
==========================================
- Hits         5941     5924      -17     
- Misses       1681     1719      +38     

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

@mairanteodoro mairanteodoro added this to the 25Q1_B16 milestone Dec 11, 2024
@mairanteodoro mairanteodoro force-pushed the hackday_mteodoro_sky_variance_readnoise branch from 37cf8b1 to 2448932 Compare December 11, 2024 14:20
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Dec 11, 2024
@mairanteodoro mairanteodoro force-pushed the hackday_mteodoro_sky_variance_readnoise branch from 60b0642 to d8dddf9 Compare December 11, 2024 21:50
@mairanteodoro mairanteodoro marked this pull request as ready for review December 12, 2024 16:03
@mairanteodoro mairanteodoro requested a review from a team as a code owner December 12, 2024 16:03
@braingram
Copy link
Collaborator

I follow the addition of the new weight type but I'm not sure I see the connection between the ticket and resampling each computed var_sky into a var_sky added to the mosaic model.

If the goal is to save var_sky in the mosaic model would you open a rad and roman_datamodels PRs to update the schema to include var_sky?

docs/roman/resample/arguments.rst Outdated Show resolved Hide resolved
romancal/resample/resample_utils.py Show resolved Hide resolved
romancal/resample/resample_utils.py Show resolved Hide resolved
romancal/resample/resample_utils.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

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

I think this looks conceptually right; thanks! Can you put a couple of example files on /grp/roman somewhere, together with an invocation (run strun roman::resample blah.asn --weight_type skyivm ??) so that I can easily poke at some example input and output files?

romancal/resample/resample_utils.py Outdated Show resolved Hide resolved
@mairanteodoro
Copy link
Collaborator Author

If the goal is to save var_sky in the mosaic model would you open a rad and roman_datamodels PRs to update the schema to include var_sky?

Yes, that's the idea.

@mairanteodoro mairanteodoro force-pushed the hackday_mteodoro_sky_variance_readnoise branch from 192c1cb to 65f7acd Compare December 17, 2024 14:35
@mairanteodoro mairanteodoro force-pushed the hackday_mteodoro_sky_variance_readnoise branch from 2064e73 to af738f3 Compare December 30, 2024 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation regression_testing testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants