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

Make sure we remove the right patch from the image viewer when removing the slit overlay #803

Merged
merged 2 commits into from
Sep 3, 2021

Conversation

astrofrog
Copy link
Collaborator

Description

While testing out #762 I ran across a bug which was that the code to remove the slit overlay was assuming things about the order and number of marks in the figure, which caused an error of the type x not in list. This PR should fix it by making sure we remove the right mark.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • Do the proposed changes follow the STScI Style Guides?
  • Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • Did the CI pass? If not, are the failures related?
  • Is a change log needed? If yes, is it added to CHANGES.rst?
  • Is a milestone set? Milestone is only currently required for PRs related to Imviz MVP.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@astrofrog astrofrog added the bug Something isn't working label Aug 24, 2021
@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #803 (1d0e35b) into main (4018090) will decrease coverage by 0.08%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #803      +/-   ##
==========================================
- Coverage   67.21%   67.12%   -0.09%     
==========================================
  Files          65       65              
  Lines        4612     4618       +6     
==========================================
  Hits         3100     3100              
- Misses       1512     1518       +6     
Impacted Files Coverage Δ
...onfigs/mosviz/plugins/slit_overlay/slit_overlay.py 51.35% <25.00%> (-4.54%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4018090...1d0e35b. Read the comment docs.

@pllim
Copy link
Contributor

pllim commented Aug 24, 2021

Hmm... @rosteen and @javerbukh should have a look given they are attempting these PRs that sound related:

@pllim pllim removed their request for review August 24, 2021 15:56
@rosteen
Copy link
Collaborator

rosteen commented Aug 31, 2021

As far as I can tell this breaks the slit overlay plugin. Unchecking the Visible box no longer removes the slit from the image, although if I uncheck it and then click another row then the new image won't have the slit displayed.

@astrofrog
Copy link
Collaborator Author

I'll investigate!

@astrofrog
Copy link
Collaborator Author

@rosteen - could you try again?

Copy link
Collaborator

@rosteen rosteen left a comment

Choose a reason for hiding this comment

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

Looks like the bug I was seeing is fixed, I can toggle slit visibility from the plugin again. Green checks on codecov would be nice, but I'm approving.

@pllim
Copy link
Contributor

pllim commented Sep 3, 2021

If this works now and unblocks the rest of the PRs, I think we should just merge. 😉

@rosteen rosteen merged commit 38db2c9 into spacetelescope:main Sep 3, 2021
@rosteen
Copy link
Collaborator

rosteen commented Sep 3, 2021

That sounded like a second approval to me XD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants