wave rms refactor #1672
Merged
wave rms refactor #1672
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
profxj
approved these changes
Sep 16, 2023
pypeit/core/wavecal/autoid.py
Outdated
| @@ -1712,6 +1724,10 @@ class HolyGrail: | |||
| islinelist : bool, optional | |||
| Is lamps a linelist (True), or a list of ions (False) | |||
| The former is not recommended except by expert users/developers | |||
| rms_thresh : float, optional | |||
| @@ -1712,6 +1724,10 @@ class HolyGrail: | |||
| islinelist : bool, optional | |||
| Is lamps a linelist (True), or a list of ions (False) | |||
| The former is not recommended except by expert users/developers | |||
| rms_thresh : float, optional | |||
| RMS threshold for the wavelength solution fit | |||
| measured_fwhms : ndarray, optional | |||
There was a problem hiding this comment.
Maybe explain what happens if it isn't provided?
pypeit/spectrographs/gemini_gnirs.py
Outdated
| @@ -322,7 +322,8 @@ def config_specific_par(self, scifile, inp_par=None): | |||
| par['calibrations']['slitedges']['fit_min_spec_length'] = 0.5 | |||
|
|
|||
| # Wavelengths | |||
| par['calibrations']['wavelengths']['rms_threshold'] = 1.0 # Might be grating dependent.. | |||
| par['calibrations']['wavelengths']['rms_thresh_frac_fwhm'] = 0.4 # Might be grating dependent.. | |||
There was a problem hiding this comment.
Am a bit surprised at how high this is, but it was before.
@jhennawi -- does this make sense to you?
There was a problem hiding this comment.
I don't know where I got 2.5 for fwhm, it's more like 4 (the default) from the dev suite. I changed that. The RMS values for 32/mm are pretty high in the Dev Suite (even before these changes), the calibration wasn't that great. See below.
pypeit/spectrographs/gemini_gnirs.py
Outdated
| @@ -354,7 +355,7 @@ def config_specific_par(self, scifile, inp_par=None): | |||
| par['calibrations']['slitedges']['sync_predict'] = 'nearest' | |||
|
|
|||
| # Wavelengths | |||
| par['calibrations']['wavelengths']['rms_threshold'] = 1.0 # Might be grating dependent.. | |||
| par['calibrations']['wavelengths']['rms_thresh_frac_fwhm'] = 1.0 # Might be grating dependent.. | |||
There was a problem hiding this comment.
same here. we should inspect the solutions for GNIRS and see if this makes sense..
There was a problem hiding this comment.
I may have forgotten to change this. I checked the values in the Dev Suite and rms is ~0.1 (fwhm ~3pixels), so I changed this to 0.05
| @@ -391,9 +392,9 @@ def default_pypeit_par(cls): | |||
|
|
|||
| # Wavelengths | |||
| # 1D wavelength solution with arc lines | |||
| par['calibrations']['wavelengths']['rms_threshold'] = 1.0 | |||
| par['calibrations']['wavelengths']['rms_thresh_frac_fwhm'] = 0.1 | |||
There was a problem hiding this comment.
should this be 0.05 given we reduced the FWHM?
There was a problem hiding this comment.
I changed to 0.05, but it does not matter much, since FIRE long uses the template method and the rms threshold is not used.
| @@ -310,7 +310,7 @@ def default_pypeit_par(cls): | |||
| par['calibrations']['wavelengths']['lamps'] = ['NeI', 'ArI', 'HgI'] | |||
| # Wavelengths | |||
| # 1D wavelength solution | |||
| par['calibrations']['wavelengths']['rms_threshold'] = 0.5 | |||
| par['calibrations']['wavelengths']['rms_thresh_frac_fwhm'] = 0.17 | |||
There was a problem hiding this comment.
Not 0.1? I'm assuming you found a higher value was better here.
There was a problem hiding this comment.
I found that the measured fwhm is more like 3 pixels (but 2x binned), therefore rms_thresh_frac_fwhm would become 0.17. The datasets that we have in the Dev Suite use the template method anyway.
Closed
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## develop #1672 +/- ##
===========================================
- Coverage 41.33% 41.09% -0.25%
===========================================
Files 189 189
Lines 43296 43544 +248
===========================================
- Hits 17896 17893 -3
- Misses 25400 25651 +251
|
|
@jhennawi I set for all the spectrographs to use the measured fwhm. The Dev Suite run is successful. These are the slits/orders that have a difference of RMS >0.1 compared to a |
jhennawi
reviewed
Oct 1, 2023
| #embed() | ||
|
|
||
| return new_bad_slits | ||
| # This routine is commented out because it is not used. |
There was a problem hiding this comment.
Shall we move to deprecated.
jhennawi
reviewed
Oct 1, 2023
| # plt.show() | ||
| # #embed() | ||
| # | ||
| # return new_bad_slits | ||
|
|
||
| def get_use_tcent_old(self, corr, cut=True, arr_err=None, weak=False): |
jhennawi
approved these changes
Oct 1, 2023
|
For the records, full Dev suite tests pass. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As titled. The parameter
rms_threshis now replaced byrms_thresh_frac_fwhm, which is expressed as a fraction of FWHM.I run the Dev Suite and it passed. I compared the wavelength calibration with these changes to a Dev Suite run in the
developbranch, see plots. What do you think?