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

Implement indicators #4

Merged
merged 2 commits into from
Nov 4, 2020
Merged

Implement indicators #4

merged 2 commits into from
Nov 4, 2020

Conversation

almarklein
Copy link
Collaborator

@almarklein almarklein commented Nov 3, 2020

This implements showing indicators for other slicers on the same volume. The proposed implementation allows keeping the API similar to what we had: you still simply create unit slicer objects. This keeps the API simple and flexible.

It works by making use of dictionary id's and pattern matching callbacks. Every component of the slicer has an id composed of:
context-id, scene-id, axis. When the scene-id is shared between slicer objects, they also share indicators. And this all works well by default, because the scene-id is derived from id(volume).

Also consider the added example, that has two slicers for the same axis, and indeed both appear in the other slicer:
afbeelding

This also includes some changes to make the low-res images display exactly in the same position as the original image.

This was referenced Nov 4, 2020
Closed
@emmanuelle emmanuelle self-requested a review November 4, 2020 15:51
Copy link
Contributor

@emmanuelle emmanuelle left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! The scene_id mechanism is very elegant, I like the fact that it's generated from a hash of the volume by default, but it's also possible to specify a value (it could be useful when having several volumes corresponding to different image modalities, but which need to be synchronized). The only thing is that with the long id names, it's quite painful to look at the callback graph in the developer tools (which you can see with debug=True). This is not specific to this project, but it might be worth submitting a PR to Dash to represent long ids in a different format (see for example plotly/dash#1179 for the last modifications of the code generating the callback graph). Feel free to merge this PR when you want. I still need to think a bit about whether we're 100% happy with having several slicer objects instead of just one but the solution with several objects seems to work pretty well and has the advantage of being able to pass different volumes (different modalities for example)

@almarklein
Copy link
Collaborator Author

The only thing is that with the long id names, it's quite painful to look at the callback graph [...]

Thanks for the tip about the callback graph, I had not discovered it yet! It seems very useful ... if it wasn't so cluttered by the long id names ;) I created #9 to track this. Using short string id's except for where we really need the dict-id's should already help a lot.

@almarklein almarklein merged commit dcd0a6a into main Nov 4, 2020
@almarklein almarklein deleted the indicators branch November 4, 2020 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants