Update LDT/DeVeny + "quality of life" changes #1712

tbowers7 · 2023-10-25T22:45:08Z

Primarily an update to LDT/DeVeny:

Default parameter changes (mainly edge tracing and wavelength solution orders)
Instrument-Specific arc line lists
Updated wavelength template for one grating

A series of small "quality of life" improvements:

Allow pypeit_chk_wavecalib to accept lists of input files.
Allow restricted wavelength range for spectral flexure cross-correlation (this required touching a number of files in order to plumb the user-specifiable parameter all the way down to the function where it is used). This feature is useful when the end(s) of the wavelength calibration are not well constrained because of arc line placement and the user doesn't want those regions to affect the flexure correction.
Update verbosity assignment in pypeit_show_2dspec.
Doc/param fix to align with code for polynomial overscan fitting; update np.polyfit() -> np.polynomial.Chebyshev.fit().

Main repo unit tests pass, will request cloud run of DevSuite following review.

================= test session starts ================
platform darwin -- Python 3.11.6, pytest-7.4.2, pluggy-1.3.0
rootdir: /Users/tbowers/d1/codes/PypeIt
configfile: setup.cfg
testpaths: pypeit/tests
plugins: doctestplus-1.0.0, astropy-0.11.0, hypothesis-6.88.1, remotedata-0.4.1, asdf-3.0.0, cov-4.1.0, filter-subpackage-0.1.2, astropy-header-0.2.2, mock-3.12.0, arraydiff-0.5.0
collected 240 items
...
==== 240 passed, 67 warnings in 408.87s (0:06:48) ====

Companion DevSuite PR (pypeit/PypeIt-development-suite#295) adds wavelength templates.

modified: pypeit/spectrographs/ldt_deveny.py

modified: pypeit/data/arc_lines/lists/Hg_DeVeny1200_lines.dat modified: pypeit/spectrographs/ldt_deveny.py

* Allow `pypeit_chk_wavecalib` to accept lists of input files If more than one WaveCalib is to be checked at once, this commit allows for the script to accept a list of input files, and simply loops the main execution over the list of input files. * Allow restricted wavelength range for flexure cross-correlation Add FlexurePar parameters ``minwave`` and ``maxwave`` that may be used to restrict the wavelength range used for the flexure cross-correlation. This is useful in the cases where the wavelength calibration _at the edges_ of the spectral range is poorly constrained, yet there is a night-sky line used in the correlation there. Other uses may be possible. The new parameters are passed to the wavelength restrictor in `pypeit.core.flexure.spec_flux_shift` as 0 and numpy.inf as the effective min and max specified values, respectively, (while the actual parameters are ``None`` if not specified). * Tweak Hg wavelength * Adjust wavecal fit parameters for LDT/DeVeny * Update verbosity assignment in show_2dspec modified: pypeit/core/flexure.py modified: pypeit/data/arc_lines/lists/Hg_DeVeny1200_lines.dat modified: pypeit/extraction.py modified: pypeit/find_objects.py modified: pypeit/par/pypeitpar.py modified: pypeit/scripts/chk_wavecalib.py modified: pypeit/scripts/show_2dspec.py modified: pypeit/spectrographs/ldt_deveny.py

The code for fitting a polynomial to the overscan region of a raw image file for subtraction only requires one parameter (order), whereas the documentation and PypeIt parameter fed into the overscan-subtraction routine say "three". This commit aligns the documentation and params with the reality of the code. Also, for the polynomial fitting, move away from the `np.polyfit()` function (deprecated in NumPy v1.4 -- way way long time ago) to the `numpy.polynomial` pagkage. In particular, use a Chebyshev polynomial for the fitting here (orthonormal basis sets can be useful for seeing where the power is). Modify the LDT/DeVeny params to use a polynomial (order=1) for fitting the overscan. This is more appropriate for this chip and EMI pickup environment than the default SavGol filter. modified: pypeit/core/procimg.py modified: pypeit/par/pypeitpar.py modified: pypeit/spectrographs/ldt_deveny.py

modified: doc/releases/1.14.1dev.rst

Neon lines routinely recovered with LDT/DeVeny These lines were originally included with #1595, but were causing various issues with DevSuite tests. Once more robust DevSuite VET tests are developed for wavelength calibration checking, we can try again with these neon lines. new file: pypeit/data/arc_lines/lists/ArI_DeVeny_lines.dat renamed: pypeit/data/arc_lines/lists/Cd_DeVeny1200_lines.dat -> pypeit/data/arc_lines/lists/CdI_DeVeny_lines.dat renamed: pypeit/data/arc_lines/lists/Hg_DeVeny1200_lines.dat -> pypeit/data/arc_lines/lists/HgI_DeVeny_lines.dat new file: pypeit/data/arc_lines/lists/NeI_DeVeny_lines.dat modified: pypeit/spectrographs/ldt_deveny.py

Add function in the template-maker file for DV5. (Update things to pathlib.Path... needed to add a force-str in one of the other functions.) modified: pypeit/core/wavecal/spectrographs/templ_ldt_deveny.py modified: pypeit/core/wavecal/templates.py modified: pypeit/data/arc_lines/reid_arxiv/ldt_deveny_500_HgCdAr.fits

codecov-commenter · 2023-10-25T23:05:00Z

Codecov Report

Attention: 56 lines in your changes are missing coverage. Please review.

Comparison is base (cca81ff) 40.96% compared to head (08d9296) 40.98%.

Files	Patch %	Lines
pypeit/scripts/chk_wavecalib.py	0.00%	23 Missing ⚠️
pypeit/spectrographs/ldt_deveny.py	4.54%	21 Missing ⚠️
pypeit/core/procimg.py	54.54%	5 Missing ⚠️
pypeit/core/wavecal/templates.py	0.00%	2 Missing ⚠️
pypeit/scripts/chk_noise_2dspec.py	0.00%	2 Missing ⚠️
pypeit/core/wavecal/wv_fitting.py	87.50%	1 Missing ⚠️
pypeit/par/pypeitpar.py	91.66%	1 Missing ⚠️
pypeit/scripts/show_2dspec.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    #1712      +/-   ##
===========================================
+ Coverage    40.96%   40.98%   +0.01%     
===========================================
  Files          190      190              
  Lines        43892    43904      +12     
===========================================
+ Hits         17982    17994      +12     
  Misses       25910    25910

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

kbwestfall

Thanks, Tim! I have a few very minor comments and typo fixes.

I think your change from a warning to an error for the pattern-noise subtraction likely points to a deeper change we should make to rawimage, but I'd like to get input from @rcooke-ast about it. I will ping him over Slack.

pypeit/core/flexure.py

kbwestfall · 2023-10-27T16:43:02Z

pypeit/core/procimg.py

-            c = np.polyfit(np.arange(osfit.size), osfit, params[0])
-            ossub = np.polyval(c, np.arange(osfit.size))
+            poly = np.polynomial.Chebyshev.fit(np.arange(osfit.size), osfit, params[0])
+            ossub = poly(np.arange(osfit.size))


I suggest that we make this more explicit. For now I would do:

if method.lower() == 'polynomial': warnings.warn('Method "polynomial" is identical to "chebyshev". Former will be deprecated.', DeprecationWarning) if method.lower() in ['polynomial', 'chebyshev']: ...

And then eventually we get rid of the polynomial option.

I added the suggested code. When I run this with method=polynomial, however, I am not seeing the DeprecationWarning (i.e., it is being filtered somewhere). I couldn't find a warning filter within PypeIt, so it may be coming from one of the imported libraries. In this case, do we change this to a msgs.warn(), or just let it be for now? I've included the same notice in the pypeitpar listing, so it will show up in the docs.

D'Oh! Ah, well. Those warnings would likely fly by unnoticed anyway. I think having the notice in the docs is likely more useful anyway!

kbwestfall · 2023-10-27T16:43:31Z

pypeit/par/pypeitpar.py

-                                'number of repeats ; for \'savgol\', set overscan_par = ' \
-                                'order, window size ; for \'median\', set overscan_par = ' \
-                                'None or omit the keyword.'
+                                '\'polynomial\', set overcan_par = order; for \'savgol\', ' \


change "polynomial" to "chebyshev"

Done. Added deprecation warning here, and also included both "chebyshev" and "polynomial" in this class's valid_overscan_methods() method to allow both for now.

pypeit/scripts/chk_wavecalib.py

pypeit/spectrographs/ldt_deveny.py

rcooke-ast

Some comments/thoughts :-)

rcooke-ast · 2023-10-27T18:47:05Z

pypeit/spectrographs/ldt_deveny.py


        # For processing the arc frame, these settings allow for the combination of
        #   of frames from different lamps into a comprehensible Master
        par['calibrations']['arcframe']['process']['clip'] = False
        par['calibrations']['arcframe']['process']['combine'] = 'mean'
-        par['calibrations']['arcframe']['process']['subtract_continuum'] = True
+        # par['calibrations']['arcframe']['process']['subtract_continuum'] = True


Is there a reason to uncomment this for the arc lines? To be fair, it's only needed in cases where arc frames are being that contain different lamp spectra. Is this the case? Looking at the dev suite, it seems that dv7 contains multiple different lamp spectra?

The continuum-subtraction is an aesthetic issue for me -- without the subtraction (and hence negative flux values), the WaveCalib QA plots are done with a log scale. For most of our users, the desired lamps are all on at the same time (e.g., HgCdAr) -- but some insist on taking separate frames for each lamp. I have instructions in our instrument manual that if arcs with individual arc lamps are used/combined, then this parameter needs to be set to True.

I should check the DV7 .pypeit file in the DevSuite -- thanks for flagging that.

Note: I fixed the DV7 .pypeit file in pypeit/PypeIt-development-suite#296.

Just FYI - I ran the dev suite, and got an error, but I suspect that you have fixed this in this PR

pypeit/spectrographs/ldt_deveny.py

Fixed typos, added "chebyshev" option for overscan subtraction with a deprecation notice for the "polynomial" option, and updated the argument help for `pypeit_chk_wavecalib`. modified: pypeit/core/flexure.py modified: pypeit/core/procimg.py modified: pypeit/par/pypeitpar.py modified: pypeit/scripts/chk_wavecalib.py

modified: doc/releases/1.14.1dev.rst

modified: pypeit/core/procimg.py modified: pypeit/par/pypeitpar.py

Cleaned up the help string for the `--mode` command-line option. modified: pypeit/scripts/chk_noise_2dspec.py

Update docs realted to both this PR and the datacube PR. NOTE: python files edited are docstring-only changes to make sphinx happy, with the following exceptions: pypeit/core/procimg.py & pypeit/spectrographs/ldt_deveny.py -- finish adding "chebyshev" In setup.cfg, added `psutil` as a [test] and [dev] dependency for use with the DevSuite (dependency added in pypeit/PypeIt-development-suite#235). new file: doc/api/pypeit.coadd3d.rst modified: doc/api/pypeit.rst modified: doc/help/pypeit_chk_noise_2dspec.rst modified: doc/help/pypeit_chk_wavecalib.rst modified: doc/include/class_datamodel_datacube.rst modified: doc/include/dependencies_table.rst modified: doc/pypeit_par.rst modified: doc/scripts/build_datacontainer_datamodels.py modified: pypeit/coadd3d.py modified: pypeit/core/datacube.py modified: pypeit/core/procimg.py modified: pypeit/spectrographs/ldt_deveny.py modified: pypeit/specutils/pypeit_loaders.py modified: setup.cfg

tbowers7 · 2023-10-31T22:55:47Z

@kbwestfall:
Updated docs related to both this PR and the datacube PR.

NOTE: python files edited in 54afea0 are docstring-only changes to make sphinx happy, with the following exceptions:
pypeit/core/procimg.py & pypeit/spectrographs/ldt_deveny.py -- finish adding the "chebyshev" overscan method.

In setup.cfg, added the package psutil as a [test] and [dev] dependency for use with the DevSuite (dependency added in pypeit/PypeIt-development-suite#235 but never updated here).

kbwestfall · 2023-10-31T23:24:59Z

@kbwestfall: Updated docs related to both this PR and the datacube PR.

Apologies. Most (all?) of these doc fixes are repeats of things waiting on merging of other PRs (e.g., #1706) ...

NOTE: python files edited in 54afea0 are docstring-only changes to make sphinx happy, with the following exceptions: pypeit/core/procimg.py & pypeit/spectrographs/ldt_deveny.py -- finish adding the "chebyshev" overscan method.

I'm being overly picky, but I noticed you re-ordered the imports in procimg.py. I recommend we try to follow PEP 8.

In setup.cfg, added the package psutil as a [test] and [dev] dependency for use with the DevSuite (dependency added in pypeit/PypeIt-development-suite#235 but never updated here).

Thanks! (But also in #1706 )

tbowers7 · 2023-10-31T23:52:55Z

I'm being overly picky, but I noticed you re-ordered the imports in procimg.py. I recommend we try to follow PEP 8.

I think the new order is in PEP8 format:

Standard library imports. (warnings)
Related third party imports. (astropy, numpy, scipy)
Local application/library specific imports. (pypeit)

Am I misinterpreting something?

Sorry about all the duplicated changes -- was just trying to follow the development workflow and make things work.

kbwestfall · 2023-11-01T00:02:54Z

I'm being overly picky, but I noticed you re-ordered the imports in procimg.py. I recommend we try to follow PEP 8.

I think the new order is in PEP8 format:
* Standard library imports.  (`warnings`)

* Related third party imports.  (`astropy`, `numpy`, `scipy`)

* Local application/library specific imports.  (`pypeit`)
Am I misinterpreting something?

No, sorry, I regretted that complaint almost immediately after sending it! I mistakenly thought of numpy as a standard lib. So, technically you're right. E.g., astropy and numpy are both 3rd party. But I think of these imports as a dependency tree with lowest dependencies first. This means I tend to group numpy, scipy, + matplotlib first, then astropy, then ..., then pypeit. I.e., astropy depends on numpy, but not vice versa.

Sorry about all the duplicated changes -- was just trying to follow the development workflow and make things work.

Not your fault! I was just bemoaning duplicated work.

rcooke-ast

Happy to approve, but it might be good to confirm that this passes the dev suite DV7 before merging. To be fair, the dev-suite fail is likely because that merge has been done into develop already, but the PypeIt one hasn't been merged.

rcooke-ast · 2023-11-03T12:11:18Z

pypeit/spectrographs/ldt_deveny.py


        # For processing the arc frame, these settings allow for the combination of
        #   of frames from different lamps into a comprehensible Master
        par['calibrations']['arcframe']['process']['clip'] = False
        par['calibrations']['arcframe']['process']['combine'] = 'mean'
-        par['calibrations']['arcframe']['process']['subtract_continuum'] = True
+        # par['calibrations']['arcframe']['process']['subtract_continuum'] = True


Just FYI - I ran the dev suite, and got an error, but I suspect that you have fixed this in this PR

tbowers7 · 2023-11-03T15:44:47Z

Happy to approve, but it might be good to confirm that this passes the dev suite DV7 before merging. To be fair, the dev-suite fail is likely because that merge has been done into develop already, but the PypeIt one hasn't been merged.

Yes, the DevSuite PR was merged into develop already, and the new DV7 .pypeit file there needs new line lists that are included in this PR.

I've requested a full cloud DevSuite run for this branch.

tbowers7 · 2023-11-03T15:48:43Z

Apart from waiting for the DevSuite results, this PR needs to wait until #1706 clears because of duplicated doc changes that need merging.

No code changes other than variable name changes to make it easier to follow what is being done in computing the RMS of the wavelength calibration solutions. Also, add instrument-specific documentation page. new file: doc/spectrographs/ldt_deveny.rst modified: doc/spectrographs/spectrographs.rst modified: pypeit/core/wavecal/autoid.py modified: pypeit/core/wavecal/wv_fitting.py modified: pypeit/spectrographs/ldt_deveny.py

rcooke-ast · 2023-11-04T12:43:30Z

Dev-suite passes 👍

modified: pypeit/spectrographs/ldt_deveny.py

modified: README.rst

modified: doc/pypeit_par.rst modified: setup.cfg

Change the default verbosity for `pypeit_show_2dspec` to 1 (which is where all the other scripts -- save `run_pypeit` -- are). This was an oversight in #1712. Also, add new "scattlight" frametype to list of ignored frametypes for LDT/DeVeny. modified: pypeit/scripts/show_2dspec.py modified: pypeit/spectrographs/ldt_deveny.py

tbowers7 added 8 commits October 19, 2023 22:30

DeVeny slit edge adjustments

bdb2cf4

modified: pypeit/spectrographs/ldt_deveny.py

Update DeVeny Hg linelist; adjust rtol and continuum_subtract

47579a7

modified: pypeit/data/arc_lines/lists/Hg_DeVeny1200_lines.dat modified: pypeit/spectrographs/ldt_deveny.py

Merge branch 'develop' into update_deveny

c4fbe37

Release Notes

705b821

modified: doc/releases/1.14.1dev.rst

kbwestfall approved these changes Oct 27, 2023

View reviewed changes

rcooke-ast reviewed Oct 27, 2023

View reviewed changes

tbowers7 added 6 commits October 27, 2023 14:51

Merge branch 'develop' into update_deveny

011ebe6

Update release notes to reflect PR comments

c8f3102

modified: doc/releases/1.14.1dev.rst

Another sweep for typos

0bff31d

modified: pypeit/core/procimg.py modified: pypeit/par/pypeitpar.py

Clarify pypeit_chk_noise_2dspec option help

2f1a53b

Cleaned up the help string for the `--mode` command-line option. modified: pypeit/scripts/chk_noise_2dspec.py

Update release notes

5c27518

tbowers7 added 2 commits November 2, 2023 10:23

Merge branch 'develop' into update_deveny

dde56fd

Merge branch 'develop' into update_deveny

195ea28

rcooke-ast approved these changes Nov 3, 2023

View reviewed changes

rcooke-ast mentioned this pull request Nov 3, 2023

HotFix: SkyRegions GUI #1718

Merged

tbowers7 added 2 commits November 3, 2023 12:46

Merge branch 'develop' into update_deveny

ef47aa1

tbowers7 added 3 commits November 6, 2023 14:00

FWHM tewak for ldt_deveny

7bce38b

modified: pypeit/spectrographs/ldt_deveny.py

Merge branch 'platescale_hotfix' into update_deveny

73928c3

More small parameter/comment tweaks

716f880

modified: pypeit/spectrographs/ldt_deveny.py

tbowers7 mentioned this pull request Nov 8, 2023

Hotfix: Platescale binning correction #1721

Merged

tbowers7 added 2 commits November 8, 2023 12:22

Merge branch 'develop' into update_deveny

a719473

Late comment from #1724

00a68af

modified: README.rst

tbowers7 mentioned this pull request Nov 9, 2023

Deprecate conda-forge; update install docs #1724

Merged

Merge branch 'develop' into update_deveny

68f53d9

tbowers7 force-pushed the update_deveny branch from 9b4d75e to 1b6404f Compare November 17, 2023 18:29

Remake docs after merge

670a644

modified: doc/pypeit_par.rst modified: setup.cfg

tbowers7 force-pushed the update_deveny branch from 1b6404f to 670a644 Compare November 17, 2023 18:33

tbowers7 merged commit 36ceb88 into develop Nov 17, 2023
23 checks passed

tbowers7 deleted the update_deveny branch November 17, 2023 19:00

tbowers7 mentioned this pull request Dec 12, 2023

pypeit_show2dspec HOTFIX #1741

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update LDT/DeVeny + "quality of life" changes #1712

Update LDT/DeVeny + "quality of life" changes #1712

tbowers7 commented Oct 25, 2023

codecov-commenter commented Oct 25, 2023 •

edited

Loading

kbwestfall left a comment

kbwestfall Oct 27, 2023

tbowers7 Oct 27, 2023

kbwestfall Oct 27, 2023

kbwestfall Oct 27, 2023

tbowers7 Oct 27, 2023

rcooke-ast left a comment

rcooke-ast Oct 27, 2023

tbowers7 Oct 27, 2023

tbowers7 Oct 30, 2023

rcooke-ast Nov 3, 2023

tbowers7 commented Oct 31, 2023

kbwestfall commented Oct 31, 2023

tbowers7 commented Oct 31, 2023

kbwestfall commented Nov 1, 2023

rcooke-ast left a comment

rcooke-ast Nov 3, 2023

tbowers7 commented Nov 3, 2023 •

edited

Loading

tbowers7 commented Nov 3, 2023

rcooke-ast commented Nov 4, 2023

Update LDT/DeVeny + "quality of life" changes #1712

Update LDT/DeVeny + "quality of life" changes #1712

Conversation

tbowers7 commented Oct 25, 2023

codecov-commenter commented Oct 25, 2023 • edited Loading

Codecov Report

kbwestfall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcooke-ast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbowers7 commented Oct 31, 2023

kbwestfall commented Oct 31, 2023

tbowers7 commented Oct 31, 2023

kbwestfall commented Nov 1, 2023

rcooke-ast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbowers7 commented Nov 3, 2023 • edited Loading

tbowers7 commented Nov 3, 2023

rcooke-ast commented Nov 4, 2023

codecov-commenter commented Oct 25, 2023 •

edited

Loading

tbowers7 commented Nov 3, 2023 •

edited

Loading