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

KCWI scattered light #1661

Merged
merged 32 commits into from
Oct 1, 2023
Merged

KCWI scattered light #1661

merged 32 commits into from
Oct 1, 2023

Conversation

rcooke-ast
Copy link
Collaborator

This PR includes the functionality to generate a (spectrograph dependent) scattered light model, and subtract this light from the input image. The scattered light should be removed before correcting for spatial and spectral illumination, so it has to be included in the rawimage class. Here is an example for the KCWI correction. This is based on the left, right, and central parts of the detector (i.e. the not illuminated parts of the detector). This is a pixelflat.

image

@codecov-commenter
Copy link

codecov-commenter commented Sep 3, 2023

Codecov Report

Merging #1661 (9c27c69) into develop (b4f86a5) will increase coverage by 0.06%.
The diff coverage is 23.91%.

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

@@             Coverage Diff             @@
##           develop    #1661      +/-   ##
===========================================
+ Coverage    41.01%   41.07%   +0.06%     
===========================================
  Files          189      189              
  Lines        43471    43583     +112     
===========================================
+ Hits         17829    17903      +74     
- Misses       25642    25680      +38     
Files Coverage Δ
pypeit/bspline/bspline.py 80.00% <ø> (ø)
pypeit/core/parse.py 55.35% <ø> (ø)
pypeit/par/pypeitpar.py 96.07% <100.00%> (+<0.01%) ⬆️
pypeit/core/flat.py 50.30% <0.00%> (ø)
pypeit/spectrographs/spectrograph.py 67.31% <33.33%> (+0.71%) ⬆️
pypeit/find_objects.py 45.95% <0.00%> (-0.49%) ⬇️
pypeit/core/datacube.py 5.58% <0.00%> (ø)
pypeit/images/rawimage.py 57.18% <16.66%> (-1.32%) ⬇️
pypeit/spectrographs/keck_kcwi.py 29.81% <22.22%> (-0.38%) ⬇️

... and 23 files with indirect coverage changes

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.

A few minor comments from me, and two questions:

  • Can you add a description of this approach to the keck_kcwi.rst doc?
  • Can we save the scattered light image somewhere? In either or both of the Flat calilbration frame and the spec2d files?

pypeit/core/parse.py Outdated Show resolved Hide resolved
pypeit/core/skysub.py Outdated Show resolved Hide resolved
pypeit/find_objects.py Show resolved Hide resolved
pypeit/images/rawimage.py Outdated Show resolved Hide resolved
pypeit/images/rawimage.py Outdated Show resolved Hide resolved
pypeit/spectrographs/keck_kcwi.py Outdated Show resolved Hide resolved
pypeit/spectrographs/keck_kcwi.py Show resolved Hide resolved
pypeit/spectrographs/spectrograph.py Outdated Show resolved Hide resolved
pypeit/spectrographs/keck_kcwi.py Show resolved Hide resolved
pypeit/core/datacube.py Show resolved Hide resolved
@kbwestfall kbwestfall mentioned this pull request Sep 11, 2023
Copy link
Collaborator

@jhennawi jhennawi left a comment

Choose a reason for hiding this comment

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

Very minor comments. Thanks!

Copy link
Collaborator Author

@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.

Thanks @jhennawi and @kbwestfall - I've addressed your comments.

In the process, I have also merged in in PR #1678 to take advantage of the new way to document changes.

pypeit/core/datacube.py Show resolved Hide resolved
pypeit/images/rawimage.py Outdated Show resolved Hide resolved
pypeit/images/rawimage.py Outdated Show resolved Hide resolved
pypeit/spectrographs/keck_kcwi.py Outdated Show resolved Hide resolved
pypeit/spectrographs/keck_kcwi.py Outdated Show resolved Hide resolved
pypeit/spectrographs/keck_kcwi.py Show resolved Hide resolved
pypeit/spectrographs/keck_kcwi.py Show resolved Hide resolved
pypeit/spectrographs/spectrograph.py Outdated Show resolved Hide resolved
pypeit/core/parse.py Outdated Show resolved Hide resolved
@rcooke-ast
Copy link
Collaborator Author

Sorry @kbwestfall, I forgot to address your other questions...

  • Can you add a description of this approach to the keck_kcwi.rst doc?

Yes, I will add this to Keck/KCWI docs.

  • Can we save the scattered light image somewhere? In either or both of the Flat calilbration frame and the spec2d files?

I think this is not a good idea for the moment. I am working on a parameterised model that would need to store the model parameters, rather than an image (together with a routine that can generate the image from the model parameters). So, I'm tempted to wait until the next round of KCWI scattered light removal to incorporate this.

@kbwestfall
Copy link
Collaborator

On saving the scattered light image, I'm fine with putting this off, but I wanted to advocate for saving the full image. In similar situations I also used to be of the opinion that it's better to save the model parameters and provide a script if people want to reconstruct the model. But I've come around to the opinion that this presents a barrier (albeit small) to people being able to assess the results. Disk space is cheap and, IMO, enabling people to quickly open the file with astropy and view it with matplotlib is worth the cost of a larger output file.

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.

Approving!

@rcooke-ast
Copy link
Collaborator Author

Tests pass
image

Copy link
Collaborator

@jhennawi jhennawi left a comment

Choose a reason for hiding this comment

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

Approved. Thanks!

@rcooke-ast rcooke-ast merged commit a82aa78 into develop Oct 1, 2023
23 checks passed
@rcooke-ast rcooke-ast deleted the kcwi_cleanse3 branch October 1, 2023 18:50
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