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

Propagate trace_npoly in ech_findobj + minor X-Shooter tweaks #1832

Merged
merged 24 commits into from
Aug 26, 2024

Conversation

freddavies
Copy link
Collaborator

Fixes a bug where the polynomial order for object trace fitting, [reduce] [[findobj]] trace_npoly, was not correctly propagated into ech_objfind_ineach_order, which lead to poor tracing in X-Shooter NIR. Fortunately, it is the only instrument that uses a higher order by default, so this bug probably did not affect many other reductions.

Additionally, I have updated a few parameters of the X-Shooter detectors to reflect the latest version of the manual, and have slightly reduced the order trimming of the NIR detector to keep some more pixels in the order overlap regions, based on visual inspection of bright object traces.

@codecov-commenter
Copy link

codecov-commenter commented Jul 26, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 14 lines in your changes missing coverage. Please review.

Project coverage is 38.22%. Comparing base (754cb17) to head (0966f40).

Files Patch % Lines
pypeit/spectrographs/vlt_xshooter.py 0.00% 13 Missing ⚠️
pypeit/display/display.py 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1832   +/-   ##
========================================
  Coverage    38.21%   38.22%           
========================================
  Files          208      208           
  Lines        48414    48419    +5     
========================================
+ Hits         18500    18506    +6     
+ Misses       29914    29913    -1     

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

Copy link
Collaborator

@kbwestfall kbwestfall left a comment

Choose a reason for hiding this comment

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

Thanks @freddavies ! I may be overblowing this, but the changes to the hard-coded detector parameters feel significant. We need to run the tests, but we should also log these changes in the doc/releases/1.16.1dev.rst doc.

I pointed to examples that have an approach to identifying lampoffflats (MOSFIRE) and how to pull detector parameters from file headers (KCWI). Is that something you have time to implement in this PR, or can we find someone that does?

@@ -444,6 +416,8 @@ def check_frame_type(self, ftype, fitstbl, exprng=None):
| (fitstbl['target'] == 'LAMP,FLAT'))
& good_dark_seq)

# TODO: Figure out how to identify lampoffflats
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could follow what @debora-pe did for MOSFIRE.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had forgotten that, actually, the lampoffflats were already being identified, but mistyped as dark frames (see the code block immediately above). So it was even easier than I was expecting!

datasec=np.atleast_1d('[:,11:2058]'), # pre and oscan are in the spatial direction
oscansec=np.atleast_1d('[:,2059:2106]'),
)
gain = np.atleast_1d(0.64), # Assumes high gain. TODO: grab this from the header
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are a few instruments that pull this kind of information from the header. The one that comes to mind is KCWI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I noticed that the binning was already being grabbed via a single line just above, so I followed that example. Let me know if you think it should be done differently.

@freddavies
Copy link
Collaborator Author

I have cleared out the TODOs that I left in the spectrograph file as requested, and have confirmed that the NIR, VIS_1x1, and UVB_1x1 setups in the dev suite pass. I will run the rest of the setups now and update once they are done.

I have added one additional change to set fwhm_fromlines = False to be the default for NIR, as one of the orders seems to often be lost otherwise due to a bad wavelength solution (the dev suite pypeit file also does this). The reason for the poor wavelength solution is unclear, since the automatically measured FWHM looks correct, but the wavelength solution in that order seems to have consistently high RMS. I think investigating that underlying problem is outside the scope of this PR, however.

Copy link
Collaborator

@debora-pe debora-pe left a comment

Choose a reason for hiding this comment

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

@freddavies thanks for fixing this bug.
I am approving, but please take into account my one comment.

pypeit/spectrographs/vlt_xshooter.py Show resolved Hide resolved
@@ -915,6 +866,10 @@ def get_detector_par(self, det, hdu=None):
"""
# Binning
binning = '1,1' if hdu is None else self.get_meta_value(self.get_headarr(hdu), 'binning')

# Grab the gain and read noise from the header.
gain = None if hdu is None else hdu[0].header['HIERARCH ESO DET OUT1 CONAD']
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here.

Copy link
Collaborator

@kbwestfall kbwestfall left a comment

Choose a reason for hiding this comment

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

Hi @freddavies. Thanks for adding the release comment, but I thought it would useful to have something more specific so that people don't have to try to look up the changes themselves by digging through the code.

I've started to do this, but in the process I realized that we may not be doing something correctly with the pixel scale. There is a pixel scale defined for each detector, but there is also the order_pixelscale function. Do these need to be consistent? And in some cases the order_pixelscale function doesn't account for the binning. Does that matter?

pypeit/spectrographs/vlt_xshooter.py Show resolved Hide resolved
pypeit/spectrographs/vlt_xshooter.py Show resolved Hide resolved
@freddavies
Copy link
Collaborator Author

@kbwestfall Good point about the order_platescale function -- it appears to be used in object finding for echelle spectrographs instead of the value in the detector dict. I have now made them consistent with the new plate_scale numbers; while they no longer have any order-to-order variation, the typical values seem to be different enough from before that it is probably better to use the new ones.

@kbwestfall kbwestfall merged commit df30133 into develop Aug 26, 2024
23 checks passed
@kbwestfall kbwestfall deleted the xsh_nir_objtrace branch August 26, 2024 17:22
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