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

Infrastructure improvements for improved IFU sims #770

Merged
merged 13 commits into from
May 15, 2024

Conversation

mperrin
Copy link
Collaborator

@mperrin mperrin commented Nov 30, 2023

Work in progress for adding infrastructure for better IFU simulations, shared between NIRSpec and MIRI classes.

This PR is heavily inspired by #691 by @patapisp, but has generalized much of the implementation to work across both NIRSpec and MIRI.

This adds a new intermediate abstract subclass JWInstrument_with_IFU, and changes the NIRSpec and MIRI classes to subclass from that. The benefit will be to allow adding IFU-specialized code shared between the two instruments, but not needed in the other three JW SIs.

That shared code includes:

  • Addition of a mode attribute to indicated imaging or IFU major mode
  • move the faster data cube function to this subclass
  • Add a .band attribute to specify IFU bandpass. The implementation and behavior differs for the two classes, following the instrument designs. For NIRSpec the band is like "G295H/F290LP", derived from the .disperser and filter attributes. For MIRI the band is an MRS sub-band, like 3A.
    • For NIRSpec, this necessitated also adding filter support for the long-pass filters. This requires an update to the data files, and the data files min version.
  • For both classes, reasonable automatic support for toggling SIAF aperture name and mode, in either direction: setting mode='IFU' explicitly will change the aperturename to a relevant IFU aperture. Setting the aperture name to an IFU aperture will invoke IFU mode. The intent is to make it seamless and as automatic as possible for the user to set up a self-consistent calculation.
  • For both classes, a lookup table attribute ._IFU_bands_cubepars, derived from current CRDS cubepar files, which gives the wavelength range, wavelength sampling, and spaxel size for all bands.
  • A method get_IFU_wavelengths() that returns the wavelength sampling for the selected band, based on the cubers values. This function takes an optional nlambda argument to specify some desired sampling across the band; by default it matches the wave step from cubepars, which can be >1000 wavelengths depending on mode.
  • Auto adjust pixel (spaxel) scale based on IFU/imaging mode, and IFU band (for MIRI)
  • For NIRSpec, in IFU mode, rotate images to match the IFUalign output orientation.
  • Do we need any such rotation for MIRI to better match the IFUalign outputs for MIRI too?
  • Consider for IFU mode updating the default pixel scale to match IFUalign output cubes? Or leave it at the actual detector pixel scale as the default? The user can always simply set the pixel scale manually to a custom value, e.g. nrs.pixelscale=0.1

Example code:
MIRI:

miri = webbpsf.MIRI()
miri.mode = 'IFU'
miri.band= '2A'
waves = miri.get_IFU_wavelengths()
cube = miri.calc_datacube_fast(waves)

NIRSpec:

nrs = webbpsf.NIRSpec()
nrs.mode = 'IFU'
nrs.disperser = 'PRISM'
nrs.filter = 'CLEAR'
waves = nrs.get_IFU_wavelengths()
cube = nrs.calc_datacube_fast(waves)

@mperrin
Copy link
Collaborator Author

mperrin commented Nov 30, 2023

FYI @patapisp @tracy-beck

@mperrin mperrin added this to the Release 1.3 milestone Feb 13, 2024
@pep8speaks
Copy link

pep8speaks commented Feb 20, 2024

Hello @mperrin, Thank you for updating !

Line 60:126: E501 line too long (155 > 125 characters)
Line 69:126: E501 line too long (192 > 125 characters)
Line 95:1: E402 module level import not at top of file
Line 96:1: E402 module level import not at top of file
Line 97:1: E402 module level import not at top of file
Line 99:1: E402 module level import not at top of file
Line 114:1: E402 module level import not at top of file
Line 125:1: E402 module level import not at top of file
Line 127:1: E402 module level import not at top of file
Line 129:1: E402 module level import not at top of file
Line 131:1: E402 module level import not at top of file

Line 10:1: E402 module level import not at top of file
Line 14:1: E402 module level import not at top of file
Line 16:1: E731 do not assign a lambda expression, use a def
Line 17:1: E731 do not assign a lambda expression, use a def
Line 18:1: E731 do not assign a lambda expression, use a def
Line 20:1: E731 do not assign a lambda expression, use a def
Line 124:1: E302 expected 2 blank lines, found 1
Line 136:28: E225 missing whitespace around operator
Line 143:28: E225 missing whitespace around operator
Line 149:5: E303 too many blank lines (2)
Line 149:31: W291 trailing whitespace
Line 152:28: E225 missing whitespace around operator
Line 161:28: E225 missing whitespace around operator
Line 169:28: E225 missing whitespace around operator
Line 178:31: E226 missing whitespace around arithmetic operator
Line 183:51: E226 missing whitespace around arithmetic operator
Line 185:1: E302 expected 2 blank lines, found 1

Line 10:1: E402 module level import not at top of file
Line 13:1: E402 module level import not at top of file
Line 15:1: E731 do not assign a lambda expression, use a def
Line 20:1: E731 do not assign a lambda expression, use a def
Line 21:1: E731 do not assign a lambda expression, use a def
Line 23:1: E731 do not assign a lambda expression, use a def
Line 68:1: E302 expected 2 blank lines, found 1

Line 16:1: E402 module level import not at top of file
Line 23:1: E266 too many leading '#' for block comment
Line 162:1: E266 too many leading '#' for block comment
Line 398:5: E722 do not use bare 'except'
Line 403:9: E722 do not use bare 'except'
Line 411:46: E721 do not compare types, use 'isinstance()'
Line 445:1: E266 too many leading '#' for block comment
Line 550:33: E203 whitespace before ':'
Line 550:50: E203 whitespace before ':'
Line 551:43: E203 whitespace before ':'
Line 551:60: E203 whitespace before ':'
Line 651:1: E266 too many leading '#' for block comment

Line 302:126: E501 line too long (148 > 125 characters)
Line 389:47: E114 indentation is not a multiple of four (comment)
Line 389:47: E116 unexpected indentation (comment)
Line 728:126: E501 line too long (152 > 125 characters)
Line 731:29: E201 whitespace after '('
Line 755:1: E266 too many leading '#' for block comment
Line 979:126: E501 line too long (133 > 125 characters)
Line 981:57: E225 missing whitespace around operator
Line 985:126: E501 line too long (165 > 125 characters)
Line 993:32: E201 whitespace after '('
Line 1005:126: E501 line too long (145 > 125 characters)
Line 1024:126: E501 line too long (134 > 125 characters)
Line 1042:82: E202 whitespace before ')'
Line 1083:52: E225 missing whitespace around operator
Line 1254:91: E231 missing whitespace after ','
Line 1254:126: E501 line too long (139 > 125 characters)
Line 1441:126: E501 line too long (200 > 125 characters)
Line 1597:126: E501 line too long (131 > 125 characters)
Line 1840:13: E731 do not assign a lambda expression, use a def
Line 1842:13: E731 do not assign a lambda expression, use a def
Line 1852:126: E501 line too long (165 > 125 characters)
Line 1875:9: E266 too many leading '#' for block comment
Line 1908:9: E266 too many leading '#' for block comment
Line 1909:9: E266 too many leading '#' for block comment
Line 1949:1: E302 expected 2 blank lines, found 1
Line 1953:5: E303 too many blank lines (2)
Line 1954:26: E201 whitespace after '('
Line 1968:5: E303 too many blank lines (2)
Line 2026:126: E501 line too long (127 > 125 characters)
Line 2051:126: E501 line too long (155 > 125 characters)
Line 2071:17: E126 continuation line over-indented for hanging indent
Line 2215:126: E501 line too long (141 > 125 characters)
Line 2357:36: E225 missing whitespace around operator
Line 2397:5: E303 too many blank lines (2)
Line 2416:5: E303 too many blank lines (2)
Line 2433:13: E265 block comment should start with '# '
Line 2436:13: E265 block comment should start with '# '
Line 2438:13: E265 block comment should start with '# '
Line 2439:13: E265 block comment should start with '# '
Line 2440:13: E265 block comment should start with '# '
Line 2441:9: E265 block comment should start with '# '
Line 2449:24: E225 missing whitespace around operator
Line 2453:65: E226 missing whitespace around arithmetic operator
Line 2610:126: E501 line too long (130 > 125 characters)
Line 2654:126: E501 line too long (130 > 125 characters)
Line 2664:126: E501 line too long (141 > 125 characters)
Line 2688:126: E501 line too long (130 > 125 characters)
Line 3100:33: E116 unexpected indentation (comment)
Line 3131:33: E221 multiple spaces before operator
Line 3203:71: E231 missing whitespace after ','
Line 3204:126: E501 line too long (145 > 125 characters)
Line 3208:9: E303 too many blank lines (2)
Line 3217:5: E303 too many blank lines (2)
Line 3233:126: E501 line too long (139 > 125 characters)
Line 3248:126: E501 line too long (145 > 125 characters)
Line 3249:126: E501 line too long (137 > 125 characters)
Line 3262:126: E501 line too long (142 > 125 characters)
Line 3270:43: E261 at least two spaces before inline comment
Line 3273:5: E303 too many blank lines (2)
Line 3304:24: E225 missing whitespace around operator
Line 3326:5: E303 too many blank lines (2)

Comment last updated at 2024-05-15 17:34:45 UTC

Copy link

codecov bot commented Feb 20, 2024

Codecov Report

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

Project coverage is 60.85%. Comparing base (b536111) to head (3e393c3).
Report is 30 commits behind head on develop.

❗ Current head 3e393c3 differs from pull request most recent head a8416a1. Consider uploading reports for the commit a8416a1 to get more accurate results

Files Patch % Lines
webbpsf/webbpsf_core.py 92.75% 5 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #770      +/-   ##
===========================================
+ Coverage    59.20%   60.85%   +1.64%     
===========================================
  Files           16       16              
  Lines         6955     6708     -247     
===========================================
- Hits          4118     4082      -36     
+ Misses        2837     2626     -211     

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

@mperrin
Copy link
Collaborator Author

mperrin commented Feb 21, 2024

Here's a general question for @patapisp @tracy-beck and anyone else thinking about IFU mode PSF characterization. IFU pixel scales are irregular, and in general not square. How should we approach this in sim PSFs for the IFU? I can imaging cases being made for trying to simulate on the native sampled spaxels, or on the pixel scale that the pipeline makes drizzled datacubes. (in principle it seems better to simulate on the native detector sampling, but those are in general not square! Would need a bunch of effort to simulate different pixel scales in the alpha and beta directions.)

My hand waved suggestion is to, for now, simulate by default at the pipeline output pixel size (and, as usual, potentially oversampled relative to that scale). That at least gives us an isotropic pixel scale, avoiding the complexity of different X and Y scales. In principle we could then handle the different X and Y scales as a distortion effect, applied by interpolation after the optical propagation calculation. Thoughts?

@mperrin mperrin mentioned this pull request Feb 22, 2024
9 tasks
@patapisp
Copy link

@mperrin, regarding pixel sizes. My thoughts are as follows:

  • I would by default use the square pixel sizes that come out after cube building. This is to my understanding already working with the infrastructure you set up for the IFU mode.
  • I have concrete plans to enable MIRI IFU PSFs on the detector level which would then have the native detector sampling. Maybe we can use the same code for NIRSpec? I have not looked into the details, but I presume the distortion model might be similar.

I am looking into the PR now to try to catch up to the new features you implemented! :)

@mperrin
Copy link
Collaborator Author

mperrin commented Apr 2, 2024

Hi @patapisp - I wanted to check in since it's been a while since we chatted. Did you happen to have any time to work more on any code for MRS in webbpsf?
Right now, I think I am inclined to try to get this PR merged in with the current functionality (after de-conflicting with the changes from other PRs already merged). And then we can leave additional functionality to further improve MRS PSF models for some later PR. If you have no objections I think that's what I will do? (Past experience has taught me that it can be more trouble to pack too much stuff into one PR...) Thanks!

@mperrin mperrin force-pushed the ifu_sim_framework branch 2 times, most recently from 32f6cc7 to 5a6158f Compare April 3, 2024 21:26
@mperrin mperrin force-pushed the ifu_sim_framework branch from 5a6158f to bcfa0ae Compare April 24, 2024 19:53
@mperrin mperrin force-pushed the ifu_sim_framework branch from bcfa0ae to 569d51e Compare May 12, 2024 13:57
@mperrin mperrin marked this pull request as ready for review May 13, 2024 19:16
@mperrin
Copy link
Collaborator Author

mperrin commented May 13, 2024

@obi-wan76 @tracy-beck the webbpsf IFU-mode PR is ready for review.

I added the new v1.3.0 data files to Box which are needed for testing this, and the tests pass here on Github so that part seems good.

I'd still like to add in some more user documentation of this, but I think that might best be left for a separate PR.

@obi-wan76
Copy link
Collaborator

The example code above works for me with the relevant files from the webbpsf-data 1.3. I tried to save the cube via the outfile option, cube = miri.calc_datacube_fast(waves, outfile = 'test_cube.fits'), however, it did not save it as a cube but a single frame with all the 4 webbpsf ext

  0  OVERSAMP      1 PrimaryHDU     107   (284, 284)   float64   
  1  DET_SAMP      1 ImageHDU       108   (71, 71)   float64   
  2  OVERDIST      1 ImageHDU       112   (284, 284)   float64   
  3  DET_DIST      1 ImageHDU       113   (71, 71)   float64  

@mperrin
Copy link
Collaborator Author

mperrin commented May 15, 2024

Nice catch on the outfile parameter not being implemented for that method. Added. Grab the latest and try again now, please.

@obi-wan76
Copy link
Collaborator

The save file is working as intended. Thanks!

Looking at some of the MIRI cubes, inside the fits file header, there are a few places that refers to the mean wavelength and the wavelength "0" that show a value of 2um, i.e., WAVELEN and WAVE0. Also the history shows that it was created with a wavefront at 2um. This seems to be very NIRSpec specific, so I am wondering if the header keywords are not being updated correctly for MIRI. I think it is only an issue with some of the header information because the wave parameter is set correctly for the MIRI calculation, from 7.51 to 8.7697um, and the keyword WVLN0000 is correct for both MIRI and NIRSpec.

@mperrin
Copy link
Collaborator Author

mperrin commented May 15, 2024

Good catch with the wavelengths in the headers. I think actually there may indeed be some bug with that. I'll take a look!

@mperrin mperrin force-pushed the ifu_sim_framework branch from 28083e8 to 62c6d66 Compare May 15, 2024 17:34
@mperrin mperrin force-pushed the ifu_sim_framework branch from 62c6d66 to a8416a1 Compare May 15, 2024 17:34
@mperrin
Copy link
Collaborator Author

mperrin commented May 15, 2024

I pushed some improvements to the wavelength handling in the calc_datacube_fast code, so (a) now it will use a more reasonable wavelength for MIRI calculations for the initial optical propagation, and (b) the redundant WAVE0 keyword is deleted, and instead there are WAVELN00, WAVELN01 etc. (consistent with the keywords from the regular calc_datacube output.)

@mperrin
Copy link
Collaborator Author

mperrin commented May 15, 2024

Should be ready for re-test now

Copy link
Collaborator

@obi-wan76 obi-wan76 left a comment

Choose a reason for hiding this comment

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

Great, it looks good now. Thanks!

@obi-wan76 obi-wan76 merged commit 1c0121d into spacetelescope:develop May 15, 2024
7 checks passed
obi-wan76 added a commit that referenced this pull request May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants