Conversation
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6770151. Configure here.
| # unbinned spatial plate scale [arcsec/pixel] | ||
| detector_platescale = None | ||
| # is the raw spectral axis reversed (wavelength decreasing)? | ||
| detector_specflip = None |
There was a problem hiding this comment.
Missing UltraDAS file extension allow-list
High Severity
INTIDSSpectrograph and its children never set allowed_extensions, so they inherit the base default of .fits / .fits.gz. INT/IDS raw frames come from UltraDAS and use the .fit extension (same as WHT/ISIS). _check_extensions therefore rejects valid IDS files before they can be read, so setup and reductions fail on real data.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 6770151. Configure here.
tbowers7
added
the
New Spectrograph
rcooke-ast
left a comment
rcooke-ast
left a comment
There was a problem hiding this comment.
This looks good to me. Approving, but please confirm tests pass, and it would be good to post an image of the wavelength QA if you have it. I will try to source some data to double check this works, as well.
| platescale = self.detector_platescale, | ||
| darkcurr = 0.0, # e-/pixel/hour; TODO: look up | ||
| saturation = 65535., | ||
| nonlinear = 0.99, # TODO: look up and update |
There was a problem hiding this comment.
Linearity seems good for both detectors 👍 we can remove the TODO here.
https://www.ing.iac.es/Engineering/detectors/ultra_eev10a.htm
| ygap = 0., | ||
| ysize = 1., | ||
| platescale = self.detector_platescale, | ||
| darkcurr = 0.0, # e-/pixel/hour; TODO: look up |
There was a problem hiding this comment.
darkcurr is 4/e-/pixel/hour for EEV10A
https://www.ing.iac.es/Engineering/detectors/ultra_eev10a.htm
kbwestfall
left a comment
kbwestfall
left a comment
There was a problem hiding this comment.
A couple of minor comments from me.
| :class:`INTIDSEEV10Spectrograph` (``int_ids_eev10``) and | ||
| :class:`INTIDSREDPLUS2Spectrograph` (``int_ids_redplus2``). | ||
|
|
||
| Generated by JXP and Claude. |
There was a problem hiding this comment.
Can we remove this from every docstring?
| ``'[1:4099,1:366]'``. | ||
| """ | ||
| xsec, ysec = section.strip().strip('[]').split(',') | ||
| return f'[{ysec.strip()},{xsec.strip()}]' |
There was a problem hiding this comment.
This is a very general function. Can it be moved to the base class?
4 participants
Adding the IDS spectrograph on the INT
A Dev-Suite PR will soon follow
Note
Low Risk
Additive new-instrument code with no changes to existing spectrograph reduction paths; risk is mainly incorrect metadata or parameters for unverified RED+2 data until dev-suite validation.
Overview
Adds PypeIt support for the INT Intermediate Dispersion Spectrograph (IDS) on the 2.54m Isaac Newton Telescope, including a new
INTtelescope entry and a fullint_idsspectrograph module.The shared
INTIDSSpectrographbase handles IDS-specific FITS metadata (RA/DEC conversion, binning fromCCDSUM, arc lamp state fromAGARCLMP), reads gain, read noise, and trim/overscan regions from headers with FITS-to-numpy axis swapping forTRIMSEC/BIASSEC, and wires frame typing offIMAGETYPplus lamp/exposure rules. Default and grating-specific parameters target CuAr/CuNe wavelength calibration (holy-grail by default; R1200B usesfull_templatewithint_ids_R1200B.fits). Two concrete spectrograph classes are registered:int_ids_eev10(verified against test data) andint_ids_redplus2(documented placeholder). Both are markedsupported = Falseuntil broader validation lands via the planned dev-suite work.Reviewed by Cursor Bugbot for commit 6770151. Bugbot is set up for automated code reviews on this repo. Configure here.