Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Debug orientation operations with multiple viewers #2759

Merged
merged 11 commits into from
Mar 20, 2024
Merged

Debug orientation operations with multiple viewers #2759

merged 11 commits into from
Mar 20, 2024

Conversation

rosteen
Copy link
Collaborator

@rosteen rosteen commented Mar 18, 2024 •
edited
Loading

This fixes a few bugs with adding, changing and deleting orientations when multiple viewers exist. I don't believe there was an issue open for these to close.

Fixes:

  • An if statement that was checking against a callback dict instead of an entry in the dict
  • Selecting a new orientation in the viewer data menu will now only change the selection in the Orientation plugin if the correct viewer is selected in the plugin.
  • Deleting an orientation in a viewer data menu will not result in Default Orientation being selected in any viewers where the deleted orientation was currently selected.

@rosteen rosteen added bug Something isn't working imviz plugin Label for plugins common to multiple configurations labels Mar 18, 2024
@rosteen rosteen added this to the 3.9 milestone Mar 18, 2024
pllim
pllim reviewed Mar 18, 2024
View reviewed changes
jdaviz/configs/imviz/plugins/orientation/orientation.py Outdated
else:
self.orientation.selected = "Default orientation"
# Need to manually trigger this here for...reasons
self._change_reference_data()
Copy link
Contributor

Choose a reason for hiding this comment

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

If "reasons" is unknown, we have to be sure this won't accidentally break #2751 😬

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At worst it will see that the selected orientation and reference data already match, but I'll rebase on top of your PR locally to double check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I rebased onto your #2751 branch and both our fixes still work.

@pllim
Copy link
Contributor

pllim commented Mar 18, 2024

Is it possible to add test for this case (and that it actually fails on main without this patch)?

@rosteen
Copy link
Collaborator Author

rosteen commented Mar 18, 2024

Is it possible to add test for this case (and that it actually fails on main without this patch)?

I'll try adding a test, should be possible.

bmorris3
bmorris3 reviewed Mar 18, 2024
View reviewed changes
jdaviz/configs/imviz/plugins/orientation/orientation.py Outdated Show resolved Hide resolved
jdaviz/app.py Outdated Show resolved Hide resolved
pllim
pllim reviewed Mar 18, 2024
View reviewed changes
jdaviz/app.py Outdated
for viewer_id in self._viewer_store:
if orientation_plugin is not None:
orientation_plugin.viewer.selected = viewer_id
orient = orientation_plugin.orientation.selected
Copy link
Contributor

Choose a reason for hiding this comment

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

Should have named this branch murder_on_the_orient_express.

@rosteen
Copy link
Collaborator Author

rosteen commented Mar 18, 2024

Is it possible to add test for this case (and that it actually fails on main without this patch)?

I added one test that fails on main (for the second bug in the description), still trying to figure out a good way to access the data menu widget to test the third bug. I may come back to that tomorrow.

pllim
pllim reviewed Mar 18, 2024
View reviewed changes
jdaviz/app.py Outdated
for viewer_id in self._viewer_store:
if orientation_plugin is not None:
orientation_plugin.viewer.selected = viewer_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't user see the viewer selection change every iteration when L2045 is run?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes...I might be able to do this more elegantly without changing things in the plugin 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Your comment made me realize I didn't need to touch the plugin at all for the fix for bug 3, updated to a much better fix. Getting a fresh pair of eyes on things helps so much sometimes.

@pllim
Copy link
Contributor

pllim commented Mar 18, 2024

Maybe it is a mistake to allow users to change ref data from front-end data menu...

pllim
pllim reviewed Mar 18, 2024
View reviewed changes
jdaviz/app.py Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented Mar 18, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 91.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 88.70%. Comparing base (9515b7a) to head (5641fde).
Report is 3 commits behind head on main.

Files Patch % Lines
...z/configs/imviz/plugins/orientation/orientation.py 80.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2759      +/-   ##
==========================================
+ Coverage   88.68%   88.70%   +0.01%     
==========================================
  Files         108      108              
  Lines       16214    16223       +9     
==========================================
+ Hits        14379    14390      +11     
+ Misses       1835     1833       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rosteen
Copy link
Collaborator Author

rosteen commented Mar 18, 2024

@pllim I added a test that fails on main for the other bug.

@rosteen @pllim
Apply suggestions from code review
5641fde 
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
pllim
pllim approved these changes Mar 18, 2024
View reviewed changes
Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM!

bmorris3
bmorris3 approved these changes Mar 20, 2024
View reviewed changes
Copy link
Contributor

@bmorris3 bmorris3 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, thanks.

@rosteen rosteen merged commit 8440a99 into spacetelescope:main Mar 20, 2024
20 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@bmorris3 bmorris3 bmorris3 approved these changes

@pllim pllim pllim approved these changes

@javerbukh javerbukh Awaiting requested review from javerbukh javerbukh is a code owner

@kecnry kecnry Awaiting requested review from kecnry kecnry is a code owner

@haticekaratay haticekaratay Awaiting requested review from haticekaratay haticekaratay is a code owner

@cshanahan1 cshanahan1 Awaiting requested review from cshanahan1 cshanahan1 is a code owner

@gibsongreen gibsongreen Awaiting requested review from gibsongreen gibsongreen is a code owner

Assignees
No one assigned
Labels
bug Something isn't working imviz plugin Label for plugins common to multiple configurations Ready for final review
Projects
None yet
Milestone
3.9
Development

Successfully merging this pull request may close these issues.
3 participants
@rosteen @pllim @bmorris3