Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

NF: Adds overlay option in OrthoSlicer3D #850

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

Draft
rmarkello wants to merge 4 commits into nipy:master
base: master
Choose a base branch
Loading
from rmarkello:enh/viewer

Conversation

Copy link
Contributor

@rmarkello rmarkello commented Dec 10, 2019
edited
Loading

Closes #752.

Adds the ability to include an overlay to a plotted OrthoSlicer3D object using the set_overlay method. Still very much a WIP (hence the draft PR!).

To Do:

  • Add alpha parameter to set_overlay method and use alpha.setter instead of calling im.set_alpha() (L388-9)
  • Confirm / fix functionality when two 4D volumes are plotted (one as underlay and one as overlay), especially when images have a different # of volumes
  • Add tests for new functionality to nibabel/tests/test_viewers.py
  • Update documentation to include OrthoSlicer3D / .orthoview()

Copy link
Contributor Author

I also have a few questions that I think will help guide the rest of the PR:

  1. Should users be able to include an overlay image directly in the call to img.orthoview() instead of having to save the returned OrthoSlicer3D object and call the set_overlay method explicitly? If so, how many of the set_overlay parameters should be exposed in the .orthoview() method?
  2. I haven't yet tackled the issue of plotting two 4D volumes. In particular, I'm not sure what to do for the fourth (volume trace) axis. It would likely be very noisy to have both traces plotted at once. We could potentially use a twinx axis instance if desired, but I'd be open to suggestions on what you think would "look best".
  3. From what I can tell there is currently no documentation on OrthoSlicer3D or the .orthoview() method in nibabel (except for in the API reference). I (personally) use this feature all the time and would be happy to add some documentation on it (including this new overlay feature), but wanted to confirm that it had not been consciously/purposefully excluded from the docs before moving forward.

Copy link

codecov bot commented Dec 10, 2019
edited
Loading

Codecov Report

Merging #850 into master will decrease coverage by 0.46%.
The diff coverage is 22.35%.

Impacted file tree graph

@@ Coverage Diff @@
## master #850 +/- ##
==========================================
- Coverage 91.84% 91.38% -0.47% 
==========================================
 Files 97 97 
 Lines 12427 12509 +82 
 Branches 2191 2208 +17 
==========================================
+ Hits 11414 11431 +17 
- Misses 678 743 +65 
 Partials 335 335 
Impacted Files Coverage Δ
nibabel/spatialimages.py 95.00% <20.00%> (-1.45%) ⬇️
nibabel/viewers.py 81.02% <22.50%> (-15.12%) ⬇️

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 516434c...6f91b92. Read the comment docs.

Copy link
Member

effigies commented Dec 10, 2019
edited
Loading

  1. Should users be able to include an overlay image directly in the call to img.orthoview() instead of having to save the returned OrthoSlicer3D object and call the set_overlay method explicitly? If so, how many of the set_overlay parameters should be exposed in the .orthoview() method?

I don't have strong opinions here. I think following matplotlib conventions for passing up additional parameters is probably going to provide the most expected (if not intuitive) interface.

  1. I haven't yet tackled the issue of plotting two 4D volumes. In particular, I'm not sure what to do for the fourth (volume trace) axis. It would likely be very noisy to have both traces plotted at once. We could potentially use a twinx axis instance if desired, but I'd be open to suggestions on what you think would "look best".

twinx looks reasonable to me. In addition, you could think about something where you extend the y-axis up ~2SD for one graph and ~2SD down for the other, so they are mostly non-overlapping. I suspect the only way to find what looks good will be to fiddle with it.

  1. From what I can tell there is currently no documentation on OrthoSlicer3D or the .orthoview() method in nibabel (except for in the API reference). I (personally) use this feature all the time and would be happy to add some documentation on it (including this new overlay feature), but wanted to confirm that it had not been consciously/purposefully excluded from the docs before moving forward.

Absolutely, it would be a great addition. And doc updates are welcome during the RC phase.

rmarkello reacted with thumbs up emoji

@effigies effigies added this to the 3.1.0 milestone Dec 10, 2019
Copy link
Member

effigies commented Apr 7, 2020

Hi @rmarkello, just checking in on this. Do you want to aim for a 3.1 release (soon-ish...) or 3.2?

@effigies effigies mentioned this pull request Apr 8, 2020
10 tasks
Copy link
Contributor Author

Hey @effigies ! Thanks for the ping, and sorry for the delayed response.

I'm happy to try and get core functionality into 3.1, but am not sure if a full docs addition would be feasible by then. Are you willing to split that into a separate PR, or would you rather I hold off on this 'till 3.2?

Copy link
Member

Quick responses are beyond me, as well. It would be good to have docs and features together, but I'm also not in a great rush to release if you think you're going to have time. So I'll leave it to you to decide. I'll try to be more prompt in responding, though my time is extremely constrained these days.

Copy link
Contributor Author

Hey @effigies! I think getting this done in the next few weeks is gonna be a bit of a stretch for me, unfortunately... I'd say go ahead and release without this and I'll try and get to it in the next ~month.

@effigies effigies modified the milestones: 3.1.0, 3.0.2, 3.2.0 Apr 17, 2020
Copy link
Member

Sounds good.

Copy link
Member

effigies commented Jul 6, 2020

Just checking back in on this one, @rmarkello. Anything I can do to help this along?

Copy link
Contributor Author

Hey @effigies ! Sorry for the radio silence—lots of deadlines over the past several months, though things are looking a bit better in the short term. I think I'll have some time in the coming weeks to work on this (famous last words). I managed to make some minor progress today (I decided to punt combo 4d under/overlay and just TypeError it—happy to discuss pros/cons to that decision if you'd like).

One question re documentation: do you mind if I create a new page that describes the OrthoSlicer3D object? From my reading it doesn't really fit into any of the current pages of the user manual, though I'm happy to take suggestions otherwise!

Sorry again for letting this sit so long.

Copy link
Member

effigies commented Aug 2, 2020
edited
Loading

(I decided to punt combo 4d under/overlay and just TypeError it—happy to discuss pros/cons to that decision if you'd like).

Since it's a data shape (rather than a Python type) issue, I'd go with ValueError. NotImplementedError might also work...

do you mind if I create a new page that describes the OrthoSlicer3D object?

Not at all. It would fit well in https://nipy.org/nibabel/manual.html, if you don't already have a good place in mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.2.0
Development

Successfully merging this pull request may close these issues.

Allow overlaying images in OrthoSlicer3D

AltStyle によって変換されたページ (->オリジナル) /