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

Echelle improvements #1693

Merged
merged 8 commits into from
Oct 16, 2023
Merged

Echelle improvements #1693

merged 8 commits into from
Oct 16, 2023

Conversation

profxj
Copy link
Collaborator

@profxj profxj commented Oct 4, 2023

Two changes in this short PR:

  1. Allow the usage of use_2dmodel_mask for echelle spectrographs.
  2. Turns off re-initializing the reduce_bpm in the final call to global_sky

The 2nd item was leading to crashes in Echelle data when an order had problems
during object finding.

No new tests or docs, but I am going to run the DevSuite.
And the new functionality has been tested on real world X-Shooter data.

@profxj profxj requested review from debora-pe and jhennawi October 4, 2023 17:56
@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2023

Codecov Report

Merging #1693 (0459298) into develop (b23ec14) will increase coverage by 0.00%.
The diff coverage is 75.00%.

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

@@           Coverage Diff            @@
##           develop    #1693   +/-   ##
========================================
  Coverage    41.05%   41.05%           
========================================
  Files          189      189           
  Lines        43568    43570    +2     
========================================
+ Hits         17888    17889    +1     
- Misses       25680    25681    +1     
Files Coverage Δ
pypeit/core/skysub.py 59.27% <ø> (ø)
pypeit/find_objects.py 46.09% <100.00%> (+0.14%) ⬆️
pypeit/pypeit.py 77.12% <100.00%> (ø)
pypeit/extraction.py 61.93% <0.00%> (-0.22%) ⬇️

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.

All looks good. I only have a small comment.
Thanks for doing this @profxj!

pypeit/find_objects.py Show resolved Hide resolved
@profxj
Copy link
Collaborator Author

profxj commented Oct 5, 2023

tests started

@profxj
Copy link
Collaborator Author

profxj commented Oct 6, 2023

Test Summary

--- PYTEST PYPEIT UNIT TESTS PASSED 239 passed, 70 warnings in 538.67s (0:08:58) ---
--- PYTEST UNIT TESTS PASSED 133 passed, 171 warnings in 990.98s (0:16:30) ---
--- PYTEST VET TESTS PASSED 58 passed, 68 warnings in 1010.66s (0:16:50) ---
--- PYPEIT DEVELOPMENT SUITE PASSED 231/231 TESTS ---
Testing Started at 2023-10-05T16:24:11.656047
Testing Completed at 2023-10-06T07:47:17.898775
Total Time: 15:23:06.242728

@profxj profxj requested review from kbwestfall and removed request for jhennawi October 16, 2023 13:50
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.

Two things: One minor comment on the docstring; and a possible bug I randomly caught that might never get triggered, but makes the code misleading. Approving anyway.

pypeit/find_objects.py Show resolved Hide resolved
pypeit/find_objects.py Show resolved Hide resolved
@profxj profxj merged commit 5473f82 into develop Oct 16, 2023
23 checks passed
@profxj profxj deleted the bad_order_fix branch October 16, 2023 19:34
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