-
Notifications
You must be signed in to change notification settings - Fork 259
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
NF: Adds overlay option in OrthoSlicer3D #850
base: master
Are you sure you want to change the base?
Conversation
I also have a few questions that I think will help guide the rest of the PR:
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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.
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.
Absolutely, it would be a great addition. And doc updates are welcome during the RC phase. |
Hi @rmarkello, just checking in on this. Do you want to aim for a 3.1 release (soon-ish...) or 3.2? |
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? |
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. |
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. |
Sounds good. |
Just checking back in on this one, @rmarkello. Anything I can do to help this along? |
Beginning of functionality to allow overlays in OrthoSlicer3D
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 One question re documentation: do you mind if I create a new page that describes the Sorry again for letting this sit so long. |
Since it's a data shape (rather than a Python type) issue, I'd go with
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. |
Closes #752.
Adds the ability to include an overlay to a plotted
OrthoSlicer3D
object using theset_overlay
method. Still very much a WIP (hence the draft PR!).To Do:
alpha
parameter toset_overlay
method and usealpha.setter
instead of callingim.set_alpha()
(L388-9)nibabel/tests/test_viewers.py
OrthoSlicer3D
/.orthoview()