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-3043 user option to select wavelength range of IFU cube not working for NIRSpec #7427

Merged

Conversation

jemorrison
Copy link
Contributor

@jemorrison jemorrison commented Jan 6, 2023

Resolves JP-3043

Closes #

This PR addresses a bug for NIRSpec IFU when trying to select the wavelength ranges to make the IFU cube. The code was not using these parameters for NIRSpec data.

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

@jemorrison jemorrison added this to the Build 9.1 milestone Jan 6, 2023
@jemorrison
Copy link
Contributor Author

jemorrison commented Jan 6, 2023

There were numerous flake error that I also fixed to make sure this went through ci testing correctly
The lines that fix the bug are 1344-1348 in ifu_cube..py

@jemorrison
Copy link
Contributor Author

I still need to run the regression tests - the system is busy now

@codecov
Copy link

codecov bot commented Jan 6, 2023

Codecov Report

Base: 78.61% // Head: 78.49% // Decreases project coverage by -0.12% ⚠️

Coverage data is based on head (6191232) compared to base (21b439e).
Patch coverage: 89.47% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7427      +/-   ##
==========================================
- Coverage   78.61%   78.49%   -0.13%     
==========================================
  Files         455      455              
  Lines       39144    39148       +4     
==========================================
- Hits        30773    30729      -44     
- Misses       8371     8419      +48     
Flag Coverage Δ
nightly 78.48% <89.47%> (-0.13%) ⬇️
unit 51.34% <5.26%> (-0.01%) ⬇️
Impacted Files Coverage Δ
jwst/cube_build/ifu_cube.py 91.56% <89.47%> (-0.13%) ⬇️
jwst/regtest/conftest.py 64.67% <0.00%> (-23.37%) ⬇️
jwst/regtest/regtestdata.py 84.86% <0.00%> (-1.38%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@drlaw1558
Copy link
Collaborator

Code looks ok I think, but a couple more general questions.

  • According to the documentation wavemin/wavemax are generally-available parameters. At the moment they don't do anything (indeed, I've never even tried), and after this PR they will do something for NIRSpec but not for MIRI.
  • What's the best approach? Change the documentation to say that these are NIRSpec-only parameters, or add the functionality for MIRI as well? Or is this already enabled in some other part of the code? (If so why is it different?)
  • Either way, we should clarify that specifying this on command line will override any custom values in a cubepar file. Is this consistent with pipeline behaviour in other steps?

@jemorrison
Copy link
Contributor Author

The min and max wavelength are supposed to work for MIRI. But I just tested with MIRI data and I get an error. Let me fix this

@jemorrison
Copy link
Contributor Author

jemorrison commented Jan 9, 2023

The min and max wavelength ranges do work for MIRI.
The code is different because of history really. The NIRSPEC team at one time did not want to rely on the min and max values set by the cubepars reference file, but wanted to use the data to set the min and max. Still think that is how cube build works - it sets the min and max from the data not the cubepars reference file FOR NIRSPEC. But I will test this.
For MIRI using min and max is just tricky because I just ran a single cal file that was going to produce 2 ifucubes (I did not specify output_type ='multi'). I gave a max and min wavelength for the second channel but since no pixels on the first channel had those wavelengths it returned an error. If I specified the channel and the wavelength range it worked fine or if I specified output_type='multi' it worked.
We should clarify the documentation to explain how to use these parameters. When a user provides a command line option, it always overrides what is in the reference files. That is how all the steps work, as I understand it.

@jemorrison
Copy link
Contributor Author

jemorrison commented Jan 10, 2023

@drlaw1558 @jmuzerolle
I have reviewed how the wavelength parameters are used for MIRI and NIRSpec.
I found a little issue. The cube pars reference file for NIRSpec does not have an entry for the drizzle weighting. For MIRI the drizzle wavelength table is used for the non-linear wavelength cases:

  1. when we combine bands together as we do in calspec2 (output_type = multi)
  2. if the user selects the output_type to be channel.

For NIRSpec the default is to make single grating/filter IFU cubes. If the user wanted to have multiple filters then I think we would run into a problem because there will not be a wavelength table read in. There is an initial min and max wavelength that is read in from the first extension of the cubepars reference file. For NIRSPEC the wavelength tables to be used for the non-linear case are read in for weighting = 'msm' or 'emsm' .

Concerning this PR and having the user supply min and max wavelengths:
in ifu_cube.determine_cube_parameters:
in both cases the user can supply a min and max wavelength to build the cube from. Setting these parameters sets up self.wavemin and self.wavemax.
In ifu_cube.setup_ifucube_wcs:
If the user has not provided a value for wavemin or wavemax for MIRI the values in the cubepars reference file is used.
If the cube has a non-linear wavelength scale then the wavelength table read in from cubepars is used.
For NIRSpec if the user has not provided a value for wavemin or wavemax then these values are derived from the data. This PR checks if the user supplied a value, then it will use those. I remember Nora L wanting the wavelength range to be determined from the data - but that was a decision made YEARS ago and it would be easier to maintain the code if both MIRI and NIRSPEC worked the same way.

**It might be TIME to clean up cube_build. @drlaw1558 do we need to keep weighting = emsm or msm. It would REALLY REALLY clean things up if we just always used drizzle.
@jmuzerolle we need to get a wavelength table for the drizzle weighting method into cube pars. I think it is only need for the non-linear wavelength case which is an off-line option for NIRSpec, but we should still get this in.

@drlaw1558
Copy link
Collaborator

@jemorrison Now that we've had a chance to test things fairly extensively with flight data, I'm inclined to agree with ditching the MSM and EMSM code. This would simplify both the code and the reference files. At the same time, I'm inclined to change NIRSpec to match the MIRI convention of setting wavelength ranges from cubepar, both for code clarity and because it's helpful to have cubes with fixed sizes. That latter isn't my call though.

We should split up this work. This ticket as it exists right now fixes the immediate issue with specifying a wavelength range, so it should probably be merged and closed. I'll open a new JP issue about the general overhaul to cube building and solicit feedback on there from NIRSpec prior to moving forward.

@jemorrison
Copy link
Contributor Author

Yes lets try and get this into the pipeline soonish (at least the development version). This will help a user with Helpdesk ticket who wanted to create NIRSpec IFUcube and select the wavelength range

CHANGES.rst Show resolved Hide resolved
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.

Looks good to me.

@hbushouse
Copy link
Collaborator

Regression test run at https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/535/ is clean. Merging.

@hbushouse hbushouse merged commit 837f31f into spacetelescope:master Jan 12, 2023
@jemorrison jemorrison deleted the cube_build_wavelength_range branch July 17, 2023 18:12
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.

None yet

3 participants