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

Adds R4K support for MDM OSMOS #1710

Merged
merged 12 commits into from
Nov 2, 2023
Merged

Adds R4K support for MDM OSMOS #1710

merged 12 commits into from
Nov 2, 2023

Conversation

profxj
Copy link
Collaborator

@profxj profxj commented Oct 21, 2023

As titled.

This detector is obsolete so I doubt there will be
much usage. But here it is anyways!

One catch -- the "raw" frames users often receive
are not raw.

@profxj profxj changed the base branch from release to develop October 21, 2023 12:45
@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2023

Codecov Report

Merging #1710 (f3425fc) into develop (6733b52) will decrease coverage by 0.03%.
The diff coverage is 26.31%.

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

@@             Coverage Diff             @@
##           develop    #1710      +/-   ##
===========================================
- Coverage    41.06%   41.04%   -0.03%     
===========================================
  Files          190      190              
  Lines        43661    43714      +53     
===========================================
+ Hits         17930    17941      +11     
- Misses       25731    25773      +42     
Files Coverage Δ
pypeit/core/wavecal/autoid.py 28.68% <ø> (ø)
pypeit/images/rawimage.py 57.18% <100.00%> (ø)
pypeit/par/pypeitpar.py 96.03% <100.00%> (ø)
pypeit/wavecalib.py 54.50% <ø> (ø)
pypeit/core/procimg.py 54.14% <11.11%> (-1.13%) ⬇️
pypeit/spectrographs/mdm_osmos.py 29.83% <26.08%> (-2.22%) ⬇️

@rcooke-ast rcooke-ast self-requested a review October 21, 2023 15:31
Copy link
Collaborator

@rcooke-ast rcooke-ast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me - only minor changes suggested.

doc/releases/1.14.1dev.rst Outdated Show resolved Hide resolved
doc/spectrographs/mdm_osmos.rst Outdated Show resolved Hide resolved
pypeit/core/procimg.py Outdated Show resolved Hide resolved
@profxj
Copy link
Collaborator Author

profxj commented Oct 21, 2023

Tests pass for both MDM detectors

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.

Minor changes suggested, but I'm approving now. Thanks!

pypeit/core/procimg.py Show resolved Hide resolved
pypeit/core/procimg.py Outdated Show resolved Hide resolved
pypeit/core/wavecal/autoid.py Show resolved Hide resolved
pypeit/images/rawimage.py Outdated Show resolved Hide resolved
pypeit/spectrographs/mdm_osmos.py Outdated Show resolved Hide resolved
the data one receives to have been
windowed. You may therefore need to
window the rest. There is a Notebook
in the DevSuite that shows an example of
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provide a link.

Return metadata for the selected detector.

THIS IS FOR WINDOWED SCIENCE FRAMES
AND WE ARE HACKING THE CALIBS
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consider providing a script that does this. I realize this is pretty specific because it's only needed for this instrument, but I imagine users would be happier calling something like pypeit_prep_data mdm_osmos_4k *.fits then having to dig up the notebook. That script can apply to more instruments as needed (e.g., we could use it to help people add the slit info from tilsotua for LRIS).

@kbwestfall kbwestfall merged commit b1843e4 into develop Nov 2, 2023
23 checks passed
@kbwestfall kbwestfall deleted the mdm_r4k branch November 2, 2023 21:06
@rcooke-ast
Copy link
Collaborator

Could this PR have produced an error on the dev suite?

image

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