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

HotFix: SkyRegions GUI #1718

Merged
merged 6 commits into from
Nov 3, 2023
Merged

HotFix: SkyRegions GUI #1718

merged 6 commits into from
Nov 3, 2023

Conversation

rcooke-ast
Copy link
Collaborator

This hotfix ensures that the calib key/dir/ID are all stored in the SkyRegions calibrations file. Several errors and bits of code cleanup are also implemented.

@rcooke-ast rcooke-ast changed the title fix error HotFix: SkyRegions GUI Nov 2, 2023
@rcooke-ast rcooke-ast requested a review from kbwestfall November 2, 2023 14:41
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.

One minor suggestion, but it looks good otherwise. This touches a core function, so should we run tests?

else:
# Build the master Sky Regions calibration frame
msskyreg = buildimage.SkyRegions(image=inmask.astype(float), PYP_SPEC=self.spectrograph)
return msskyreg
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is a bit clearer:

if not self._use_updates:
    return None

# Generate the mask
inmask = skysub.generate_mask(self.pypeline, self._skyreg, self.slits, self.slits_left,
                              self.slits_right)
if np.all(np.logical_not(inmask)):
    msgs.warn("Sky regions are empty - master calibration frame will not be generated")
    return None

# Build the master Sky Regions calibration frame
return buildimage.SkyRegions(image=inmask.astype(float), PYP_SPEC=self.spectrograph)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed - I've made that change!

@rcooke-ast
Copy link
Collaborator Author

This touches a core function, so should we run tests?

Yes, we probably should - running now...

@rcooke-ast
Copy link
Collaborator Author

Two dev suite fails, but these are unrelated to this PR. One is fixed by #1710, and the other is fixed by #1712.

image

Copy link
Collaborator

@profxj profxj left a comment

Choose a reason for hiding this comment

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

all good, modulo the master bit
time to scrub that from our past :)

pypeit/core/gui/skysub_regions.py Outdated Show resolved Hide resolved
pypeit/core/gui/skysub_regions.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 3, 2023

Codecov Report

Merging #1718 (8346112) into develop (b1843e4) will decrease coverage by 0.01%.
The diff coverage is 9.09%.

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

@@             Coverage Diff             @@
##           develop    #1718      +/-   ##
===========================================
- Coverage    41.04%   41.03%   -0.01%     
===========================================
  Files          190      190              
  Lines        43714    43724      +10     
===========================================
+ Hits         17941    17942       +1     
- Misses       25773    25782       +9     
Files Coverage Δ
pypeit/core/skysub.py 59.16% <50.00%> (-0.11%) ⬇️
pypeit/scripts/skysub_regions.py 10.52% <0.00%> (-1.02%) ⬇️
pypeit/core/gui/skysub_regions.py 10.46% <7.14%> (+0.16%) ⬆️

@rcooke-ast rcooke-ast merged commit f7973cd into develop Nov 3, 2023
23 checks passed
@rcooke-ast rcooke-ast deleted the hotfix_skysub branch November 3, 2023 19:42
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