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 vispy axes labels #5565
Fix vispy axes labels #5565
Conversation
@brisvag : assigning you because of your recent overlay refactor work. @psobolewskiPhD : would be great if you could also take a look and help with testing to make sure this covers all the issues you found! |
Codecov Report
@@ Coverage Diff @@
## main #5565 +/- ##
==========================================
+ Coverage 87.93% 89.43% +1.49%
==========================================
Files 609 611 +2
Lines 51238 51280 +42
==========================================
+ Hits 45055 45860 +805
+ Misses 6183 5420 -763
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@@ -17,7 +17,6 @@ def __init__(self, *, viewer, overlay, parent=None) -> None: | |||
super().__init__( | |||
node=Axes(), viewer=viewer, overlay=overlay, parent=parent | |||
) | |||
self.overlay.events.visible.connect(self._on_visible_change) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is unrelated to the bug fix, but I picked it up along the way.
The base overlay already connects to the visible
event of the overlay model. I'm not sure if the extra connection does any harm, but there's no reason for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested all the axis re-ordering, plus 3D, plus renaming.
Everything looks correct.
To me the code change makes sense 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't suggest cause it's too far from the diff, but I would then remove the call to self._on_labels_text_change()
from self.reset()
, since it's now duplicated by self._on_data_change()
Thanks for the reviews both! I'll probably merge this shortly after the weekend unless there are any objections.
Good catch. Done. |
@@ -311,7 +311,7 @@ def test_surface_normals(): | |||
faces = np.array([[0, 1, 2], [3, 4, 5], [6, 7, 8]]) | |||
values = np.array([1, 2, 3, 1, 2, 3, 1, 2, 3]) | |||
|
|||
normals = dict(face=dict(visible=True, color='red')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how that passed through, but it should be now fixed in main as well from #5575
…apari into fix-5536-axes-display-order
As #4907 has the 0.4.18 milestone, this should too. |
# Description This ensures that the vispy axes overlay labels are updated when the general data of the overlay changes. One common way this happens is as a result of a change to `Dims.ndim` which may occur when layers are added or removed. I added a test to cover the bug in #5536. This relies on the same utility function I have used previously to get the vispy node scene size, so I brought that into the general test utilities. I also added a few smaller unit tests for the vispy overlay as sanity checks before I found the actual issue. ## Type of change - [x] Bug-fix (non-breaking change which fixes an issue) # References Closes #5536 --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
# Description This ensures that the vispy axes overlay labels are updated when the general data of the overlay changes. One common way this happens is as a result of a change to `Dims.ndim` which may occur when layers are added or removed. I added a test to cover the bug in #5536. This relies on the same utility function I have used previously to get the vispy node scene size, so I brought that into the general test utilities. I also added a few smaller unit tests for the vispy overlay as sanity checks before I found the actual issue. ## Type of change - [x] Bug-fix (non-breaking change which fixes an issue) # References Closes #5536 --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
# Description This ensures that the vispy axes overlay labels are updated when the general data of the overlay changes. One common way this happens is as a result of a change to `Dims.ndim` which may occur when layers are added or removed. I added a test to cover the bug in #5536. This relies on the same utility function I have used previously to get the vispy node scene size, so I brought that into the general test utilities. I also added a few smaller unit tests for the vispy overlay as sanity checks before I found the actual issue. ## Type of change - [x] Bug-fix (non-breaking change which fixes an issue) # References Closes #5536 --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
# Description This ensures that the vispy axes overlay labels are updated when the general data of the overlay changes. One common way this happens is as a result of a change to `Dims.ndim` which may occur when layers are added or removed. I added a test to cover the bug in #5536. This relies on the same utility function I have used previously to get the vispy node scene size, so I brought that into the general test utilities. I also added a few smaller unit tests for the vispy overlay as sanity checks before I found the actual issue. ## Type of change - [x] Bug-fix (non-breaking change which fixes an issue) # References Closes #5536 --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
# Description This ensures that the vispy axes overlay labels are updated when the general data of the overlay changes. One common way this happens is as a result of a change to `Dims.ndim` which may occur when layers are added or removed. I added a test to cover the bug in #5536. This relies on the same utility function I have used previously to get the vispy node scene size, so I brought that into the general test utilities. I also added a few smaller unit tests for the vispy overlay as sanity checks before I found the actual issue. ## Type of change - [x] Bug-fix (non-breaking change which fixes an issue) # References Closes #5536 --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Description
This ensures that the vispy axes overlay labels are updated when the general data of the overlay changes. One common way this happens is as a result of a change to
Dims.ndim
which may occur when layers are added or removed.I added a test to cover the bug in #5536. This relies on the same utility function I have used previously to get the vispy node scene size, so I brought that into the general test utilities.
I also added a few smaller unit tests for the vispy overlay as sanity checks before I found the actual issue.
Type of change
References
Closes #5536