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-3547: Flux conservation for spectral resampling #8596

Merged
merged 31 commits into from
Jul 26, 2024

Conversation

melanieclarke
Copy link
Collaborator

@melanieclarke melanieclarke commented Jun 24, 2024

Resolves JP-3547
Resolves JP-3544
Resolves JP-3328

Closes #8297
Closes #8293
Closes #7790

Several fixes associated with spectral flux conservation through resampling. Also added missing documentation for spectral resampling modules.

For NIRSpec:

  • create the output WCS by resampling linearly in spatial offset from center rather than in slit units. This makes the output spatial area the same as the input by default.
  • fix the output WCS for moving targets in spec3

For MIRI:

  • reverse the sense of the scaling when pixel_scale_ratio is provided, so that output_scale = input_scale / pixel_scale_ratio, as documented.
  • fix a bug in array sizing when pixel_scale_ratio is not 1.0
  • do not apply pixel_scale_ratio to the wavelength array

For both:

  • implement support for a directly specified pixel_scale
  • scale the output pixel area keywords when pixel_scale or pixel_scale_ratio are set
  • if the input data is flux density instead of surface brightness, additionally scale the flux in the input images before resampling if pixel_scale or pixel_scale_ratio are set.

Checklist for PR authors (skip items if you don't have permissions or they are not applicable)

  • 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
  • All comments are resolved
  • Make sure the JIRA ticket is resolved properly

Copy link

codecov bot commented Jun 24, 2024

Codecov Report

Attention: Patch coverage is 95.06726% with 11 lines in your changes missing coverage. Please review.

Project coverage is 60.47%. Comparing base (771c1c0) to head (3e3d718).

Files with missing lines Patch % Lines
jwst/resample/resample_spec.py 94.56% 10 Missing ⚠️
jwst/resample/resample.py 95.83% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8596      +/-   ##
==========================================
+ Coverage   60.16%   60.47%   +0.30%     
==========================================
  Files         370      369       -1     
  Lines       38667    38408     -259     
==========================================
- Hits        23265    23226      -39     
+ Misses      15402    15182     -220     

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

@melanieclarke
Copy link
Collaborator Author

@melanieclarke melanieclarke marked this pull request as ready for review July 3, 2024 21:22
@melanieclarke melanieclarke requested a review from a team as a code owner July 3, 2024 21:22
@melanieclarke melanieclarke requested review from mcara and stscirij July 3, 2024 21:23
@melanieclarke
Copy link
Collaborator Author

melanieclarke commented Jul 5, 2024

Regression tests showed a problem with data centering for some spec3 data sets (e.g. in s2d products for test_nirspec_mos_fs_spec3). I pushed a fix and started another run here:
https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1589/

@melanieclarke melanieclarke force-pushed the jp-3547 branch 3 times, most recently from 23b945f to fe21641 Compare July 15, 2024 18:22
@melanieclarke
Copy link
Collaborator Author

Restarted regression tests here:
https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1606/

There were some unrelated failures for reference file changes in the last run.

@melanieclarke melanieclarke requested review from tapastro and nden July 15, 2024 19:05
@melanieclarke
Copy link
Collaborator Author

I think this is ready for review now. It would be helpful if @nden or @mcara could review from the WCS perspective. @stscirij, could you check on the documentation updates, and see if the note about updating the extraction aperture if the pixel parameters change is sufficient?

@mcara
Copy link
Member

mcara commented Jul 25, 2024

Is there anything for spectral data that is equivalent to output_model.meta.photometry.pixelarea_steradians. Shouldn't this be adjusted accordingly if pixel_ratio != 0?

Yes, that's handled at the end of the init function for ResampleSpecData.

I don't see it.

Yes, has been added to this PR but it was not in the original (on master) code.

@mcara
Copy link
Member

mcara commented Jul 25, 2024

I think I finally figured out the math that justifies why the scaling must be sqrt(pix_tatio) and now I have a better understanding of this. However, according to this newly acquired understanding of the problem, it seems to me that something similar must be done for the spectral coordinate too, unless it is guaranteed that all input images are on the same wavelength grid and this will be the grid used for the output image. For example, if one resamples two non-NIRSPEC input images whose spectral grid is (for example):

image 1: 500.0, 501.0, 502.0, 503.0, 504.0, ...
image 2: 500.5, 501.5, 502.5, 503.5, 504.5, ...

then, the resampled image' grid along the spectral axis will be, thanks to build_interpolated_output_wcs():

output image spectral axis grid: 500.0, 500.5, 501.0, 501.5, 502.0, 502.5, 503.0, 503.5, 504.0, ...

Also see

if resample_utils.is_sky_like(
self.input_models[0].meta.wcs.output_frame
):
if self.input_models[0].meta.instrument.name != "NIRSPEC":
self.output_wcs = self.build_interpolated_output_wcs()
else:
self.output_wcs = self.build_nirspec_output_wcs()
else:
self.output_wcs = self.build_nirspec_lamp_output_wcs()
)

So, if data are not in units of spectral density (i.e., MJ/m), the pixel ratio along the spectral axis (0.5 in the above example) should also be taken into account when computing iscale.

At first glance NIRSPEC data are not affected by this unless user-provided output_wcs has a finer-sampled spectral grid. For the case of self.input_models[0].meta.instrument.name != "NIRSPEC", I do not know what the units of the data are. So please verify that they are in density, then nothing needs to be done and if not - then a correction needs to be applied to the iscale.


One more issue: reported pixel area as computed/reported in

if output_pix_area is not None:
self.blank_output.meta.photometry.pixelarea_steradians = output_pix_area
self.blank_output.meta.photometry.pixelarea_arcsecsq = (
output_pix_area * np.rad2deg(3600)**2)

may be incorrect. In the resampled image, the grid along the spatial axis is a grid along the slit and it is a 1D thing. There is no "area" here at first glance but in reality, each "pixel" along the spectral axis was integrated in the direction perpendicular to the slit. So, I wonder whether self.blank_output.meta.photometry.pixelarea_steradians should be multiplied by slit width. Of course this is not needed if nominal_area = self.input_models[0].meta.photometry.pixelarea_steradians already takes into account slit width.

@melanieclarke
Copy link
Collaborator Author

So please verify that they are in density, then nothing needs to be done and if not - then a correction needs to be applied to the iscale.

The units are in density, in the spectral axis. Jy is a flux density unit (10^−26 W m−2 Hz−1). Only the spatial area changes need to be accounted for for flux conservation.

@melanieclarke
Copy link
Collaborator Author

So, I wonder whether self.blank_output.meta.photometry.pixelarea_steradians should be multiplied by slit width. Of course this is not needed if nominal_area = self.input_models[0].meta.photometry.pixelarea_steradians already takes into account slit width.

I wrestled with that too, developing this. The nominal_area, set in the photom step, takes into account the slit width, which is not otherwise recorded anywhere in the data model (that I could find).

I initially thought we might need to apply a direct update to the nominal area based on the WCS, the way you did for imaging, but eventually realized it wasn't possible for this reason. The nominal area includes the slit width, the WCS does not. That's why my changes to the resample_spec init function are structured the way they are: pixel_ratio can relatively correct the nominal area, for one of the spatial axes, but we can't compute a whole new spatial area from the WCS.

Copy link
Member

@mcara mcara left a comment

Choose a reason for hiding this comment

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

I am satisfied with provided responses and proposed changes make sense. Thanks!

@melanieclarke
Copy link
Collaborator Author

Thanks so much for your thoughtful review @mcara! I really appreciate you taking the time to work it through with me.

Copy link
Contributor

@tapastro tapastro left a comment

Choose a reason for hiding this comment

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

Looks good! Just one question, for my education.

@tapastro tapastro merged commit 7d97170 into spacetelescope:master Jul 26, 2024
27 checks passed
@melanieclarke melanieclarke deleted the jp-3547 branch July 26, 2024 13:10
melanieclarke added a commit to melanieclarke/jwst that referenced this pull request Jul 30, 2024
melanieclarke added a commit to melanieclarke/jwst that referenced this pull request Jul 31, 2024
melanieclarke added a commit to melanieclarke/jwst that referenced this pull request Aug 2, 2024
melanieclarke added a commit to melanieclarke/jwst that referenced this pull request Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants