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

Fix Orientation selection/creation with no loaded data #2789

Merged
merged 9 commits into from
Apr 11, 2024

Conversation

rosteen
Copy link
Collaborator

@rosteen rosteen commented Apr 8, 2024

This improves the orientation code to catch the error and raise a snackbar warning if an orientation is added before loading data into the viewer, and stops the backend from trying to set bounds to inf if an orientation is selected in the data menu before data is loaded.

@rosteen rosteen added bug Something isn't working imviz labels Apr 8, 2024
@rosteen rosteen added this to the 3.9.1 milestone Apr 8, 2024
@github-actions github-actions bot added the plugin Label for plugins common to multiple configurations label Apr 8, 2024
Copy link

codecov bot commented Apr 8, 2024

Codecov Report

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

Project coverage is 88.77%. Comparing base (f1ba4a6) to head (68684c5).
Report is 26 commits behind head on main.

Files Patch % Lines
...z/configs/imviz/plugins/orientation/orientation.py 73.33% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2789      +/-   ##
==========================================
+ Coverage   88.72%   88.77%   +0.04%     
==========================================
  Files         110      110              
  Lines       16353    16547     +194     
==========================================
+ Hits        14509    14689     +180     
- Misses       1844     1858      +14     

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

@rosteen
Copy link
Collaborator Author

rosteen commented Apr 8, 2024

I'll add test coverage later today.

Comment on lines 296 to 299
set_on_create=True, wrt_data=None):
set_on_create=True, wrt_data=None, from_ui=False):
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a publicly exposed method, is there any user-friendly name we can use here? Maybe just require_data? Or can we instead wrap with a try/except at the vue_* level that converts errors into snackbars and leaves the public methods alone?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm. I could also make add_orientation just pass the public-facing args to a private _add_orientation that has the new keyword for use by the vue_ method. I'll think about the best way to do this.

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 moved the new keyword arg to a private method.

@@ -332,6 +341,12 @@ def add_orientation(self, rotation_angle=None, east_left=None, label=None,
# default rotation:
wrt_data = self.viewer.selected_obj.first_loaded_data
if wrt_data is None: # Nothing in viewer
msg = "Viewer must have data loaded to add an orientation."
if from_ui:
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I prefer more explicit name like raise_exception=False. From UI is just one of many possible reason why someone might want or not want an exception raised. But I'll leave it up to you.

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'm not too worried about the semantics since this isn't exposed in the public API, it's only in the private method that we use internally now. I think I'll leave it as-is until a more general reason to raise an error here comes up. Thanks for the thought.

Copy link
Contributor

@javerbukh javerbukh left a comment

Choose a reason for hiding this comment

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

Fixes the original problem and I wasn't able to break it further, nice work!

Copy link
Contributor

@gibsongreen gibsongreen 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 to me! Ready for merging

@rosteen rosteen merged commit deef749 into spacetelescope:main Apr 11, 2024
15 of 16 checks passed
@pllim
Copy link
Contributor

pllim commented Apr 16, 2024

This does not have a backport but milestoned to 3.9.1. Please backport or update the milestone. Thanks!

gibsongreen pushed a commit to gibsongreen/jdaviz that referenced this pull request Apr 16, 2024
…e#2789)

* Raise error or snackbar if adding an orientation with no data loaded

* Don't update with infinite bounds

* Codestyle

* Add changelog

* Adding tests

* Load data after orientation switch

* Codestyle

* Move new keyword to private method

* Fix indents
@rosteen rosteen added the 💤 backport-v3.9.x on-merge: backport to v3.9.x label Apr 16, 2024
@rosteen
Copy link
Collaborator Author

rosteen commented Apr 16, 2024

Manual backport at #2808.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working imviz plugin Label for plugins common to multiple configurations 💤 backport-v3.9.x on-merge: backport to v3.9.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants