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

spectrum-2d-viewer: set custom toolbar tools #954

Merged
merged 1 commit into from
Dec 3, 2021

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Nov 2, 2021

Description

This pull request sets the toolbar tools on the spectrum-2d-viewer (in both mosviz and specviz2d) to match those in spectrum-viewer (1d and 2d pan/zoom, but only xrange region selection). This was a blocker for #776 and confirms that that is still a remaining issue even with xrange subsets.

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)?

* to match those in spectrum-viewer (1d and 2d pan/zoom) but only xrange region selection
@kecnry kecnry added the no-changelog-entry-needed changelog bot directive label Nov 2, 2021
@kecnry kecnry added this to the 2.1 milestone Nov 2, 2021
@github-actions github-actions bot added the mosviz label Nov 2, 2021
@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #954 (5ceac8d) into main (26563fd) will decrease coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #954      +/-   ##
==========================================
- Coverage   69.02%   68.90%   -0.12%     
==========================================
  Files          69       69              
  Lines        4907     4908       +1     
==========================================
- Hits         3387     3382       -5     
- Misses       1520     1526       +6     
Impacted Files Coverage Δ
jdaviz/configs/mosviz/plugins/viewers.py 74.60% <100.00%> (+0.20%) ⬆️
...configs/default/plugins/data_tools/file_chooser.py 65.71% <0.00%> (-3.43%) ⬇️

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 26563fd...5ceac8d. Read the comment docs.

@rosteen
Copy link
Collaborator

rosteen commented Nov 8, 2021

We should probably wait to merge this until the xrange tool in the 2D viewer doesn't break everything. Ok, not everything, but after using that tool in the 2D viewer it no longer works in the 1D viewer either.

@kecnry
Copy link
Member Author

kecnry commented Nov 8, 2021

@rosteen - is that a separate bug from #776?

@kecnry
Copy link
Member Author

kecnry commented Nov 8, 2021

After investigating further, it seems that using xrange in the 2d spectrum viewer is leaving the selection tool on "no selection (create new)" whereas the behavior after creating a subset in the 1d spectrum viewer is to immediately select the new subset. Once the xrange in the 2d spectrum viewer is used (actually used to create a subset, not just enabling the tool), this behavior breaks in the 1d spectrum viewer as well.

I do think this is separate from #776, but looks very similar to #810 (as moving the selection is creating new subsets since it is not changing from "no selection").

@rosteen
Copy link
Collaborator

rosteen commented Nov 8, 2021

Agreed that it looks very similar to #810 which, side note, should have been closed since the bug was fixed on main. I think it was resolved by a glue-jupyter fix.

Edit: might have been fixed by @astrofrog in glue-viz/glue-jupyter#256 or glue-viz/glue-jupyter#251, can't recall off the top of my head.

@pllim
Copy link
Contributor

pllim commented Dec 3, 2021

So... is this okay to merge?

@kecnry
Copy link
Member Author

kecnry commented Dec 3, 2021

Yep, I think we decided that this doesn't introduce any new bugs and the syncing issues are out of scope as they also exist in main. So as long as @rosteen doesn't have any objections, I'll consider that as a second approval and merge before end of day.

@rosteen
Copy link
Collaborator

rosteen commented Dec 3, 2021

Go for it.

@kecnry kecnry merged commit 7085176 into spacetelescope:main Dec 3, 2021
@kecnry kecnry deleted the spec2d-toolbar-tools branch December 3, 2021 20:52
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.

4 participants