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

Generate L3 exposure times from resampled exposure time images #959

Merged
merged 6 commits into from
Nov 1, 2023

Conversation

schlafly
Copy link
Collaborator

@schlafly schlafly commented Oct 27, 2023

Currently we generate L3 exposure times as sums of the times of the input exposures. This is awkward since one can imagine having two different level 3 images:

  1. 4 non-overlapping images that together cover the L3 image
  2. 4 completely overlapping images that cover the L3 image

The current code gives an exposure time for both of these L3 images that is 4x the L2 image exposure time. But in case (1) each individual pixel only sees 1x exposure time, so it's weird to say that its exposure time is 4x nominal.

This PR changes that computation so that case (1) gets 1x and case (2) gets 4x, reflecting the actual times the typical pixel sees. It does this by generating a resampled exposure time image that reflects the amount of time each part of the output image sees from input images, and then computing some simple statistics on that exposure time image.

We could consider faster resampling modes to produce these images, but just reusing the existing resampling infrastructure was expedient. This does add another resampling step, though, and so makes things ~25% slower.

Resolves RCAL-696

Regression tests are failing. This is partially expected; the exposure time in the metadata changes intentionally because of this PR, and local runs of the regression test confirm that that change gets picked up and identified as a 'regression.' I also get complaints that the data, context, err, var_rnoise, var_poisson are different. That's also occurring on main, though, and isn't related to this PR. It bears further investigation.

Checklist

  • added entry in CHANGES.rst under the corresponding subsection
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below. How to run regression tests on a PR

@schlafly schlafly added this to the 24Q1_B12 milestone Oct 27, 2023
@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Files Coverage Δ
romancal/resample/resample.py 78.01% <96.15%> (+2.57%) ⬆️

📢 Thoughts on this report? Let us know!.

@schlafly schlafly marked this pull request as ready for review October 30, 2023 14:03
@schlafly schlafly requested a review from a team as a code owner October 30, 2023 14:03
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.

Besides my small nit it looks good.

total_exposure_time = 0.0
m = exptime_tot > 0
if np.any(m):
total_exposure_time = np.median(exptime_tot[m])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we should label the median exposure time as the total exposure time.
Would is make sense to add an attribute median_exposure_time?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, changing this is part of the L3 metadata updates; I was just filling up columns. OTOH, the exposure time being computed is the total exposure time for the median pixel with at least some exposure time, so it's not totally crazy---but I agree that we should update the metadata. Do you think that should be part of this PR?

Copy link
Collaborator

@ddavis-stsci ddavis-stsci Oct 30, 2023

Choose a reason for hiding this comment

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

I don't think this is urgent.

I'm not too worried about that we can define it this way. I'm more concerned that some automated process will look for total_exposure_time and generate dubious results.

We should probably have a larger discussion with others, INS, SE?, archive?, before making a decision and finalizing the attributes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this would need a larger discussion with the other components, INS, SE?, Archive?
So I'd go ahead and merge this and write a follow up ticket.

@schlafly schlafly merged commit 630aa98 into spacetelescope:main Nov 1, 2023
27 checks passed
@schlafly schlafly deleted the resample-exptime branch November 1, 2023 19:23
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.

2 participants