JP-4285: Cube Build updates #10433
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10433 +/- ##
==========================================
+ Coverage 86.40% 86.61% +0.20%
==========================================
Files 373 373
Lines 40112 39927 -185
==========================================
- Hits 34658 34582 -76
+ Misses 5454 5345 -109 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I running the regression test to see if I need to fix any tests. |
|
@drlaw1558 One other thing while we are cleaning up the interface for cube build. Currently the parameter 'linear_wave' only works for NIRSpec data. This allows the user to switch between linear and non-linear wavelengths dimensions. The default value is True. For MIRI data, cube_build looks at the data that is to make the cube and decides if it should be linear or non-linear. In general, ifu cubes of a single band are linear and multi channel or multi band cubes can be non-linear. Cubes from spec2 are non-linear and cubes from spec3 are linear. But the user can run cube_build offline and create non-linear cube from spec3 associations by selecting the bands or channels for the cube. |
melanieclarke
left a comment
There was a problem hiding this comment.
This is a very nice simplification of the code!
Can you please add some unit tests for the expected filenames in various cases?
Also, I think we will need a breaking change note, since this removes parameters from the spec.
|
Currently the calwebb_spec2.py file sets output_type = 'multi' for MIRI MRS data and for NIRSpec it is set to 'band' |
|
@melanieclarke I wrote a unit test for testing suffix names but it uses from unittest.mock import MagicMock |
|
@melanieclarke Changed the test to use monkey patch. |
|
@melanieclarke changed the test to use monkey patch. |
melanieclarke
left a comment
There was a problem hiding this comment.
Tests look good, thanks! A few other minor clean up items, but we're getting close.
| ``pipeline [integer]`` | ||
| ``cube_build`` can be called from :ref:`calwebb_spec2 <calwebb_spec2>`, :ref:`calwebb_spec3 <calwebb_spec3>`, or stand-alone. The output IFU cubes | ||
| have a different name depending which pipeline calls it. This parameter defaults to 3 which follows the rules | ||
| for creating cubes based on the :ref:`calwebb_spec3 <calwebb_spec3>` pipeline and is also the behavior when running | ||
| ``cube_build`` stand-alone. When the :ref:`calwebb_spec2 <calwebb_spec2>` pipeline calls | ||
| ``cube_build``, it sets ``pipeline=2``. When running stand-alone or in | ||
| :ref:`calwebb_spec3 <calwebb_spec3>` pipeline, the parameter ``pipeline=3`` is set for ``cube_build``. |
There was a problem hiding this comment.
Looks like this section still needs to be removed.
There are also some comments below about the "pipeline rules" for output_type that I think need to be updated.
| """ | ||
| Unit test for Cube Build testing testing determining the correct suffix for the filename. | ||
| """ |
There was a problem hiding this comment.
| """ | |
| Unit test for Cube Build testing testing determining the correct suffix for the filename. | |
| """ | |
| """Test output filename suffixes for the cube_build step.""" |
|
|
||
|
|
||
| def test_define_cubename_suffix_miri_1band(cube_instance): | ||
| """Test MIRI suffix logic by cube_instance attributes.""" |
There was a problem hiding this comment.
| """Test MIRI suffix logic by cube_instance attributes.""" | |
| """Test MIRI suffix logic for one band and channel.""" |
|
|
||
|
|
||
| def test_define_cubename_suffix_miri(cube_instance): | ||
| """Test MIRI suffix logic for multiple bands and channels""" |
There was a problem hiding this comment.
| """Test MIRI suffix logic for multiple bands and channels""" | |
| """Test MIRI suffix logic for multiple bands and channels.""" |
|
|
||
| suffix = cube_instance.define_cubename_suffix() | ||
|
|
||
| # Assert (MIRI logic sorts subchannels according to increasing wavelength) |
There was a problem hiding this comment.
| # Assert (MIRI logic sorts subchannels according to increasing wavelength) | |
| # MIRI logic sorts subchannels according to increasing wavelength |
|
|
||
|
|
||
| def test_define_cubename_suffix_nirspec(cube_instance): | ||
| """Test NIRSpec logic fir 1 grating and 1 band .""" |
There was a problem hiding this comment.
| """Test NIRSpec logic fir 1 grating and 1 band .""" | |
| """Test NIRSpec logic for 1 grating and 1 band .""" |
There was a problem hiding this comment.
Checking coverage locally, it looks like there are a few more cases we could add here:
- NRS with a duplicate grating name to remove
- NRS with multiple gratings
- NRS internal_cal
| def define_cubename_suffix(self): | ||
| """ | ||
| Define the base output name. | ||
| Determine the filename suffix name consisting of channels/sub channels or gratings/filters. |
There was a problem hiding this comment.
| Determine the filename suffix name consisting of channels/sub channels or gratings/filters. | |
| Determine the filename suffix consisting of channels/sub channels or gratings/filters. |
There was a problem hiding this comment.
Can you please also add a breaking change note, since we're removing step parameters?
See comment below:needs update to calspec2 parameter reference file.
Resolves JP-4285
Closes #10362
This PR addresses how output filenames are created in cube_build. The 'pipeline' parameter was removed from cube along with the option to create 'single' output_type cubes. The 'single' type cubes had been used early in the mission in outlier detection to try and flag bad pixels. In the mode outlier detection called 'cube_build' to create a set of cube for each band. Each input file was mapped to the single band, all the single band s3d files where median combined and blotted back to the detector frame where bad pixels were flagged. The method of outlier detection for IFU data was replaced did not flag bad pixels well and was replaced soon after launch. The hooks to make this types of 'single' ifu cubes was left in cube_build. This pr removed the unnecessary code.
For calspec2 the internal 'step' routines create the default IFU cube names.
For calspec3 cube - cube build uses the asn name and information on the bands to create a string for the output name.
REMAINING ISSUE update to Parameter reference file.
In cube_build the 'output_type' has now been defaulted to 'band'. Only when running calspec2 on MIRI data do
we want the 'output_type' parameter = 'multi'
To run the calspec2 pipeline on miri data - the parameter reference file needs to be updated to
set 'output_type' = multi.
We could hard code it in the calwebb_spec2 code - but it is probably better to have a parameter reference file.
For miri the current calwebb_spec3 parameter reference file is set to
class: jwst.cube_build.cube_build_step.CubeBuildStep
name: cube_build
parameters: {output_type: band, skip: false}
If NIRSPec wants cubes other than 'band' from calspec3 then they should also create a parameter reference file
for calspec3.
Tasks
Build 12.0(use the latest build if not sure)no-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see changelog readme for instructions)changes/<PR#>.breaking.rstnews fragmentdocs/pageRegression tests passing: https://github.com/spacetelescope/RegressionTests/actions/runs/24732557544
okify_regteststo update the truth files