Minor improvements to quicklook, coadd2d as well as a bug fix #1821

jhennawi · 2024-06-29T04:45:52Z

This PR is preliminary and includes changes from the combine refactor PR#1813 so wait until that clear. I'm just posting it now so that @debora-pe can look at the coadd2d changes.

pypeit/coadd2d.py

kbwestfall

Looks good. Mostly minor comments from me. Please add some short notes in the release doc (doc/releases/1.16.1dev.rst) about these updates. And we should run tests!

pypeit/coadd2d.py

pypeit/par/pypeitpar.py

pypeit/scripts/ql.py

kbwestfall · 2024-07-02T22:11:56Z

pypeit/scripts/setup_coadd2d.py

+        cfg['coadd2d']['spat_samp_fact'] = par['coadd2d']['spat_samp_fact'] \
+                if args.spat_samp_fact is None else args.spat_samp_fact
+
+        # TODO JFH Why are exclude_slits and only_slits set here when they are parameters in the parset?


Good question. Assuming I was the one that wrote this, my guess is that I was mimicking the fact that only_lists and exclude_slits are keyword arguments of CoAdd2D. I'm fine with having them removed as explicit keyword arguments and included as part of cfg instead.

The method below (CoAdd2D.default_par()) is creating the default parameters and there is an interplay between exclude_slits, only_slits, and other default params. I think, what we can do is to just merge/add the lines above to the CoAdd2D.default_par() method.

Do we want to deal with this now?

No, we'll leave it for another PR.

…he object used

…jun24 # Conflicts: # pypeit/images/buildimage.py

…nto lris_jun24

codecov-commenter · 2024-07-19T12:34:14Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 11.11111% with 120 lines in your changes missing coverage. Please review.

Project coverage is 38.07%. Comparing base (3d26a0b) to head (0c27950).

Files with missing lines	Patch %	Lines
pypeit/coadd2d.py	9.37%	87 Missing ⚠️
pypeit/scripts/ql.py	0.00%	15 Missing ⚠️
pypeit/scripts/show_2dspec.py	0.00%	11 Missing ⚠️
pypeit/scripts/setup_coadd2d.py	0.00%	4 Missing ⚠️
pypeit/core/coadd.py	0.00%	3 Missing ⚠️

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

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1821      +/-   ##
===========================================
- Coverage    38.09%   38.07%   -0.02%     
===========================================
  Files          211      211              
  Lines        49002    49051      +49     
===========================================
+ Hits         18665    18675      +10     
- Misses       30337    30376      +39

Flag	Coverage Δ
	`38.07% <11.11%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

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

…nto lris_jun24

debora-pe

Looks good to me. I just have one comment. Still, let's run the tests.

pypeit/coadd2d.py

kbwestfall

Changing my review to "Request changes" until the comments are addressed and we run tests.

…jun24

debora-pe · 2024-09-19T22:04:01Z

Tests pass! Note that the PYTEST UNIT TESTS failures are still due to a problem of my machine with setup_gui. The keck_deimos/900ZD_M_6000 failure was probably a glitch in Google Drive since I re-run it without changing anything and it passed.

Test Summary
--------------------------------------------------------
--- PYTEST PYPEIT UNIT TESTS PASSED  256 passed, 62 warnings in 374.80s (0:06:14) ---
--- PYTEST UNIT TESTS FAILED  7 failed, 145 passed, 168 warnings in 5952.81s (1:39:12) ---
--- PYTEST VET TESTS PASSED  62 passed, 100 warnings in 5457.14s (1:30:57) ---
--- PYPEIT DEVELOPMENT SUITE FAILED 1/247 TESTS  ---
Failed tests:
    keck_deimos/900ZD_M_6000 pypeit
Skipped tests:
Testing Started at 2024-09-18T13:50:53.456748
Testing Completed at 2024-09-19T04:35:23.424708
Total Time: 14:44:29.967960

./pypeit_test reduce -i keck_deimos -s 900ZD_M_6000
Running reduce tests.
Running tests on the following instruments:
    keck_deimos

Reducing data from keck_deimos for the following setups:
    900ZD_M_6000

 0 passed/ 0 failed/ 0 skipped STARTED keck_deimos/900ZD_M_6000 pypeit
 1 passed/ 0 failed/ 0 skipped PASSED  keck_deimos/900ZD_M_6000 pypeit
--- PYPEIT DEVELOPMENT SUITE PASSED 1/1 TESTS  ---

Test Summary
--------------------------------------------------------
--- PYPEIT DEVELOPMENT SUITE PASSED 1/1 TESTS  ---
Testing Started at 2024-09-19T11:39:00.760448
Testing Completed at 2024-09-19T11:54:02.169846
Total Time: 0:15:01.409398

debora-pe · 2024-09-19T22:04:59Z

@kbwestfall this is ready to be merged. Please, take a look and see if you agree with it.

kbwestfall

Just a couple clean-up requests and a question about whether we deal with some keyword arguments now or later.

pypeit/core/coadd.py

pypeit/scripts/ql.py

kbwestfall · 2024-09-19T23:31:01Z

pypeit/scripts/setup_coadd2d.py

+        cfg['coadd2d']['spat_samp_fact'] = par['coadd2d']['spat_samp_fact'] \
+                if args.spat_samp_fact is None else args.spat_samp_fact
+
+        # TODO JFH Why are exclude_slits and only_slits set here when they are parameters in the parset?


Do we want to deal with this now?

debora-pe · 2024-09-20T21:36:25Z

I'm merging...unless anybody objects.

Fixed a bug in coadd2d where the platescale was not using the binning

da47015

jhennawi requested review from kbwestfall and debora-pe June 29, 2024 04:45

kbwestfall changed the base branch from develop to combine_refactor July 1, 2024 15:26

VascoSch92 reviewed Jul 2, 2024

View reviewed changes

pypeit/coadd2d.py Show resolved Hide resolved

kbwestfall approved these changes Jul 2, 2024

View reviewed changes

jhennawi added 8 commits July 4, 2024 18:55

trying to improve show_2dspec detector handling

2fc4708

minor changes to coadd2d

d4b51a3

The SNR report in multislit coadding now reports the spat_pixpos of t…

f3d7a2d

…he object used

The SNR report in multislit coadding now reports the spat_pixpos of t…

7541c1f

…he object used

The SNR report in multislit coadding now reports the spat_pixpos of t…

27e33b5

…he object used

Minor modifications.

13402bc

Merge branch 'develop' of https://github.com/pypeit/PypeIt into lris_…

d00670b

…jun24 # Conflicts: # pypeit/images/buildimage.py

Merge branch 'combine_refactor' of https://github.com/pypeit/PypeIt i…

992a393

…nto lris_jun24

Merge branch 'combine_refactor' of https://github.com/pypeit/PypeIt i…

6f38314

…nto lris_jun24

debora-pe approved these changes Jul 19, 2024

View reviewed changes

pypeit/coadd2d.py Show resolved Hide resolved

Base automatically changed from combine_refactor to develop July 22, 2024 19:34

kbwestfall requested changes Jul 22, 2024

View reviewed changes

jhennawi and others added 6 commits July 31, 2024 18:32

Merge branch 'develop' of https://github.com/pypeit/PypeIt into lris_…

9c7e9b3

…jun24

Merge branch 'develop' into lris_jun24

a368a4d

doc update

6825f1b

Merge branch 'develop' of https://github.com/pypeit/PypeIt into lris_…

a31667c

…jun24

comments

2e1ae70

user_objid

2a074b2

doc update

7a7bb85

kbwestfall approved these changes Sep 19, 2024

View reviewed changes

small cleanup

05065ca

added to realease doc

0c27950

debora-pe merged commit 2b53647 into develop Sep 20, 2024
18 checks passed

debora-pe deleted the lris_jun24 branch September 20, 2024 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor improvements to quicklook, coadd2d as well as a bug fix #1821

Minor improvements to quicklook, coadd2d as well as a bug fix #1821

jhennawi commented Jun 29, 2024

kbwestfall left a comment

kbwestfall Jul 2, 2024

debora-pe Sep 17, 2024

kbwestfall Sep 19, 2024

debora-pe Sep 20, 2024 •

edited

Loading

codecov-commenter commented Jul 19, 2024 •

edited

Loading

debora-pe left a comment

kbwestfall left a comment

debora-pe commented Sep 19, 2024

debora-pe commented Sep 19, 2024

kbwestfall left a comment

kbwestfall Sep 19, 2024

debora-pe commented Sep 20, 2024

Minor improvements to quicklook, coadd2d as well as a bug fix #1821

Minor improvements to quicklook, coadd2d as well as a bug fix #1821

Conversation

jhennawi commented Jun 29, 2024

kbwestfall left a comment

Choose a reason for hiding this comment

kbwestfall Jul 2, 2024

Choose a reason for hiding this comment

debora-pe Sep 17, 2024

Choose a reason for hiding this comment

kbwestfall Sep 19, 2024

Choose a reason for hiding this comment

debora-pe Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Jul 19, 2024 • edited Loading

Codecov Report

debora-pe left a comment

Choose a reason for hiding this comment

kbwestfall left a comment

Choose a reason for hiding this comment

debora-pe commented Sep 19, 2024

debora-pe commented Sep 19, 2024

kbwestfall left a comment

Choose a reason for hiding this comment

kbwestfall Sep 19, 2024

Choose a reason for hiding this comment

debora-pe commented Sep 20, 2024

debora-pe Sep 20, 2024 •

edited

Loading

codecov-commenter commented Jul 19, 2024 •

edited

Loading