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

JP-3465: Use TMEASURE in resample step #8212

Merged
merged 7 commits into from
Mar 6, 2024

Conversation

emolter
Copy link
Collaborator

@emolter emolter commented Jan 23, 2024

Resolves JP-3465

Closes #8073

This PR adjusts the way exposure time weighting is computed in the resample step to use the new measurement_time keyword from JP-3449 instead of the exposure time.

Jenkins job

Checklist for maintainers

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • Make sure the JIRA ticket is resolved properly

Copy link

codecov bot commented Jan 23, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 75.38%. Comparing base (4cc0ac1) to head (2791f42).
Report is 16 commits behind head on master.

Files Patch % Lines
jwst/resample/resample_utils.py 81.81% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8212      +/-   ##
==========================================
+ Coverage   75.15%   75.38%   +0.22%     
==========================================
  Files         470      474       +4     
  Lines       38604    38937     +333     
==========================================
+ Hits        29014    29351     +337     
+ Misses       9590     9586       -4     
Flag Coverage Δ *Carryforward flag
nightly 77.33% <ø> (-0.07%) ⬇️ Carriedforward from 4cc0ac1

*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.

@hbushouse hbushouse changed the title first draft JP-3465: Use TMEASURE in resample step Jan 25, 2024
@hbushouse hbushouse added this to the Build 10.2 milestone Jan 25, 2024
@emolter emolter marked this pull request as ready for review January 26, 2024 14:37
@emolter emolter requested a review from a team as a code owner January 26, 2024 14:37
Copy link
Collaborator

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

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

Code updates look good to me. Just one minor suggestion for the change log entry. I think we could go ahead and merge these changes even before you're done making updates to regtest data files. Any test inputs that don't have TMEASURE in them yet should just fall back to using EFFEXPTIM.

@hbushouse
Copy link
Collaborator

I wonder if we should make an update to the resample/resample_spec docs to just mention which exposure time related keyword is actually used by the step when doing "exptime" weighting. There may not be any mention of it at all in the docs right now, but given how many different flavors of time-related keywords we have in the headers, I think it's worth explicitly saying in the docs which one is used.

@emolter
Copy link
Collaborator Author

emolter commented Mar 4, 2024

Starting new regtest run after removal of TEXPTIME keyword. Note stdatamodels install was overridden to use this branch.

@hbushouse
Copy link
Collaborator

The latest regression test run results look good. All failures are just the absence of TEXPTIME in combined products, as expected, and a few instances of different values for the new TMEASURE keyword. So that looks good.

@hbushouse hbushouse merged commit 911b5c6 into spacetelescope:master Mar 6, 2024
29 checks passed
@emolter emolter deleted the JP-3465 branch March 6, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image resample weighting should use new time keyword
2 participants