HIRES wavelengths and a bit more #1628
Conversation
…hires_waves_and_refactor
…hires_waves_and_refactor
…hires_waves_and_refactor
…hires_waves_and_refactor
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## develop #1628 +/- ##
===========================================
+ Coverage 41.01% 41.02% +0.01%
===========================================
Files 189 189
Lines 43076 43185 +109
===========================================
+ Hits 17666 17718 +52
- Misses 25410 25467 +57
|
| self.wv_calib = self.build_wv_calib( | ||
| self.arccen, self.par['method'], | ||
| skip_QA=skip_QA, | ||
| prev_wvcalib=prev_wvcalib) | ||
|
|
||
| # Fit 2D? | ||
| if self.par['echelle']: | ||
| # Assess the fits |
There was a problem hiding this comment.
Can this control flow be moved to self.echelle_2dfit? It seems out of place in the run method. Also, please think carefully about whether these operations can be broken off into core methods that are functions or self-contained simple classes. I realize that all the modular methods are interacting for echelle wavelength calibration, but please also understand that you are carrying out core functionality in a very complicated class, whereas the model is that base classes like wavecalib are just supposed to wrap core methods.
There was a problem hiding this comment.
I agree this was too much for run()
But it doesn't fit into echelle_2dfit() which does at it says.
The code here is to redo individual echelle orders. So, I've
moved that code out of run() and into an apt named method
of BuildWaveCalib.
It mainly is control flow + calls to core methods, although still
not elegant..
Someday we need to do a major refactor of wavecalib.py
(together with autoid.py). Will be a painful day, but definitely
needed..
There was a problem hiding this comment.
Okay for now maybe just add a TODO that this control flow needs to be consolidated in the future.
|
Failing a few tests now (mainly related to the removal of Test Summary--- PYTEST PYPEIT UNIT TESTS PASSED 232 passed, 73 warnings in 361.67s (0:06:01) --- |
|
Almost there.. Test Summary--- PYTEST PYPEIT UNIT TESTS PASSED 232 passed, 73 warnings in 374.94s (0:06:14) --- |
| That is, each order must satisfy the following: | ||
|
|
||
| .. code-block:: ini | ||
|
|
There was a problem hiding this comment.
My suggestion is that we instead define:
rms_pixels = rms_threshold*FWHM
Where FWHM is either the default FWHM or the measured FWHM, i.e. it will be the latter when the automatic FWHM determination is turned on.
There was a problem hiding this comment.
Here is what we are trying to handle:
- For a given spectrograph, we have a default
rms_threshold(pixels) corresponding to a defaultfwhm(pixels) which we tune from the DevSuite. Both of these depend on slit width and binning - Now, consider a new exposure with different slit width and/or binning. The sensible thing is to scale
rms_thresholdby the measured fwhm. This works relatively well but is not perfect. Nothing ever is for wavelengths.. This is what is currently implemented in the code and described in the docs.
The fwhm_fromlines parameter only refers to whether to use the measured_fwhm when finding arclines.
It doesn't play a role rms_threshold.
There was a problem hiding this comment.
Please add an issue to updated the docs and spectrograph files to the convention that we discussed, i.e using rms_threshold_frac_fwhm. That need not be done for this PR, but clearly should be done asap.
Test Summary--- PYTEST PYPEIT UNIT TESTS PASSED 232 passed, 73 warnings in 378.98s (0:06:18) --- |
|
Relaunching tests, hopefully for the last time |
Test Summary--- PYTEST PYPEIT UNIT TESTS PASSED 232 passed, 73 warnings in 521.81s (0:08:41) --- |
This significantly advances the HIRES wavelength solutions
and deals with
rms_thresholdfor other echelle spectrographs.I am directing this PR to the open
sensfuncbranch but willredirect to
developonce that is merged. It is ready now,however, for a review.