ENH: Add manual value limits to OrthoSlicer3D/SpatialImages.orthoview #491

effigies · 2016-10-16T02:53:20Z

Setting constant value limits for time series and spatial displays can be useful for comparing two images side-by-side.

Edit: Slicing has been removed from this PR in favor of #550.

effigies · 2016-10-16T03:04:07Z

If this seems reasonable, what about adding a subslice parameter to OrthoViewer3D that takes an object that can be passed to ndarray.__getitem__? That would take care of #489 without further disruption to SpatialImage.

coveralls · 2016-10-16T03:05:53Z

Coverage decreased (-0.004%) to 95.943% when pulling 3193506 on effigies:orthoview_vlim into ec4567f on nipy:master.

codecov-io · 2016-10-16T03:14:56Z

Codecov Report

❗ No coverage uploaded for pull request base (master@41e126a). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master     #491   +/-   ##
=========================================
  Coverage          ?   94.29%           
=========================================
  Files             ?      177           
  Lines             ?    24369           
  Branches          ?     2616           
=========================================
  Hits              ?    22979           
  Misses            ?      916           
  Partials          ?      474

Impacted Files	Coverage Δ
nibabel/spatialimages.py	`96.71% <100%> (ø)`
nibabel/tests/test_spatialimages.py	`99.67% <100%> (ø)`
nibabel/viewers.py	`97.07% <100%> (ø)`
nibabel/tests/test_viewers.py	`98.78% <100%> (ø)`

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 41e126a...be7fd47. Read the comment docs.

matthew-brett · 2016-10-18T15:41:03Z

Good idea. Can you add some tests?

I know this adds some duplication, but it would be good to write out the arguments specifically, to avoid **kwargs - it makes the function signature less useful.

Maybe make these keyword only?

effigies · 2016-10-19T11:42:19Z

Looks like keyword-only is Python 3+.

Exposed axes in SpatialImage.orthoview as well, though I can remove that if that's unwanted.

coveralls · 2016-10-19T11:52:21Z

Coverage increased (+0.001%) to 95.949% when pulling 362c835 on effigies:orthoview_vlim into ec4567f on nipy:master.

matthew-brett

Adding a subspace or slicer argument sounds reasonable to me. What do you think?

matthew-brett · 2016-10-19T19:10:48Z

nibabel/viewers.py

@@ -61,6 +61,9 @@ def __init__(self, data, affine=None, axes=None, title=None):
        title : str or None, optional
            The title to display. Can be None (default) to display no
            title.
+        vlim : array-like or None, optional


What do you think of also allowing strings like 10% 90% as input, meaning percentile? Then the default could be vlim=('1%', '99%').

That works for clim, but ylim's default is a bit more complicated.

Aha. I see there's a heuristic for the colorbar around line 205 - is that what you mean?

How about throwing that away and just using the data limits as set by vlim? As in something like:

if vlim is None: vlim = ('1%', '99%')

etc.

I believe the purpose of that heuristic is so that the time-series is guaranteed to fit in the pane, with a slight buffer on top and bottom for visibility. If we set ylim(1%, 99%), then some values won't fit in the pane, by default.

Won't that be true for any not-default limits that we set? I mean, don't we need to apply the heuristic for any limits?

I'm not sure I fully understand this comment.

The three use-cases I see, myself, are (1) exploration, (2) comparison, (3) figure-generation. I think the existing default behavior is good for (1). I think the proposed changes are good for (2)+(3), where consistency/predictability is more valuable than smart estimates.

The problem I see in your proposed change is that you're changing the default behavior to something a little less friendly by setting ylim == clim even in the default case.

Apologies if I'm not making much sense.

Sure - I agree that the ylim == clim thing is bad in the default case, the question is what to do in the not-default case.

For example - I guess that people will want to use vlim to get better contrast in the images, but they don't intend to change the time-series pane. Do you think that's right? In which case, should ylim be another input argument, that defaults to the original heuristic, regardless of vlim?

@effigies - any comments here?

Hey @matthew-brett. Sorry, busy times.

To clarify my usage when I was working on this: I would have multiple files with different ranges. By setting vlim, I would be able to directly compare them, both the images and the time series. So for my case, I definitely think keeping them the same was preferable.

However, if what we want is a separate ylim, we can do that. In which case, should we change vlim to clim to drive home that one is for images, one is for time series? I chose vlim initially specifically to abstract from y/c.

What do you think?

effigies · 2016-10-19T19:53:32Z

Pushed a couple commits. Will add tests when I get another minute.

coveralls · 2016-10-19T20:34:37Z

Coverage decreased (-0.002%) to 95.945% when pulling df4fefc on effigies:orthoview_vlim into ec4567f on nipy:master.

coveralls · 2016-10-19T22:07:47Z

Coverage increased (+0.02%) to 95.963% when pulling 0f56724 on effigies:orthoview_vlim into ec4567f on nipy:master.

coveralls · 2016-10-20T01:06:31Z

Coverage increased (+0.02%) to 95.963% when pulling 7d2b9b3 on effigies:orthoview_vlim into ec4567f on nipy:master.

matthew-brett

Sorry to be slow to review.

matthew-brett · 2016-11-26T18:43:29Z

nibabel/spatialimages.py

+        axes : tuple of mpl.Axes or None, optional
+            3 or 4 axes instances for the 3 slices plus volumes,
+            or None (default).
+        vlim : array-like or None, optional


Copy vlim docstring entry from OrthoSlicer3D?

matthew-brett · 2016-11-26T19:02:10Z

nibabel/viewers.py

@@ -61,6 +61,9 @@ def __init__(self, data, affine=None, axes=None, title=None):
        title : str or None, optional
            The title to display. Can be None (default) to display no
            title.
+        vlim : array-like or None, optional


Aha. I see there's a heuristic for the colorbar around line 205 - is that what you mean?

How about throwing that away and just using the data limits as set by vlim? As in something like:

if vlim is None: vlim = ('1%', '99%')

etc.

matthew-brett · 2016-11-26T20:11:08Z

PR to this branch at effigies#1

matthew-brett · 2017-01-12T18:40:46Z

@effigies - any thoughts on my PR?

effigies · 2017-01-12T18:42:06Z

I had some when I first saw it, but was in the midst of writing. Will have another look when I get a chance.

matthew-brett · 2017-02-26T03:51:29Z

@effigies - any chance of an update here?

effigies · 2017-02-28T22:14:31Z

Oh, and one other point about ylim is that it specifically backs off a bit from the min/max in order to include the entire line on screen, a bit away from the border. effigies#1 eliminates this default behavior, which I think is bad.

matthew-brett · 2017-03-03T23:38:02Z

Just to clarify, as I return to this:

clim is a tuple setting the minimum and maximum of the whole 3D image for the purpose of the image display. It's useful to set this when futzing with the image contrast;
ylim is a name that could be used for the minimum and maximum values to display on the 4th time-series plane

At the moment, in this PRvlim changes clim, and ylim. clim can be changed using the clim property, but not ylim. The set of ylim is different in the case where vlim is default (None), because the default allows some margin at the top and bottom of the y axis, to make the line easier to see. Is that about right?

effigies · 2017-03-06T18:46:22Z

Hi Matthew. Sorry about the delay. Yes, these statements are all correct.

coveralls · 2017-07-07T17:18:41Z

Coverage increased (+0.01%) to 96.241% when pulling 8a17f08 on effigies:orthoview_vlim into f5dec18 on nipy:master.

nibotmi · 2017-10-06T19:14:41Z

☔ The latest upstream changes (presumably #562) made this pull request unmergeable. Please resolve the merge conflicts.

nibotmi · 2018-06-02T23:05:28Z

☔ The latest upstream changes (presumably #550) made this pull request unmergeable. Please resolve the merge conflicts.

nibotmi · 2018-11-29T12:43:00Z

☔ The latest upstream changes (presumably #695) made this pull request unmergeable. Please resolve the merge conflicts.

Pass kwargs to OrthoSlicer3D from SpatialImage.orthoview()

TEST: Test vlim functionality

matthew-brett reviewed Oct 19, 2016

View reviewed changes

effigies force-pushed the orthoview_vlim branch from ae0a828 to df4fefc Compare October 19, 2016 20:21

effigies force-pushed the orthoview_vlim branch from df4fefc to 0f56724 Compare October 19, 2016 21:53

effigies force-pushed the orthoview_vlim branch from 0f56724 to 7d2b9b3 Compare October 20, 2016 00:53

effigies changed the title ~~ENH: Add manual value limits to OrthoSlicer3D~~ ENH: Add manual value limits and slicing to OrthoSlicer3D/SpatialImages.orthoview Oct 20, 2016

matthew-brett reviewed Nov 26, 2016

View reviewed changes

effigies force-pushed the orthoview_vlim branch from 8a17f08 to 2cd0c2d Compare October 6, 2017 03:34

effigies changed the title ~~ENH: Add manual value limits and slicing to OrthoSlicer3D/SpatialImages.orthoview~~ ENH: Add manual value limits to OrthoSlicer3D/SpatialImages.orthoview Oct 6, 2017

effigies force-pushed the orthoview_vlim branch from 2cd0c2d to 5fec3c6 Compare October 6, 2017 19:17

effigies force-pushed the orthoview_vlim branch 2 times, most recently from 4e2b751 to 35bb818 Compare October 24, 2017 18:17

effigies force-pushed the orthoview_vlim branch from 35bb818 to 30e5464 Compare October 25, 2017 13:32

effigies force-pushed the orthoview_vlim branch 2 times, most recently from 689796a to 80466d4 Compare November 23, 2017 14:11

effigies force-pushed the orthoview_vlim branch 3 times, most recently from 4b441d6 to af8f3ec Compare December 20, 2017 14:56

effigies force-pushed the orthoview_vlim branch from af8f3ec to 3e68b3e Compare February 19, 2018 20:00

effigies force-pushed the orthoview_vlim branch from 3e68b3e to be7fd47 Compare June 2, 2018 23:10

effigies and others added 4 commits May 25, 2019 19:22

ENH: Add manual value limits to OrthoSlicer3D

7178864

Pass kwargs to OrthoSlicer3D from SpatialImage.orthoview()

DOC: Use explicit args in SpatialImage.orthoview

91d3000

TEST: Test vlim functionality

ENH: Enable percentile value limits

148c074

TEST Test percentiles

0cd1411

effigies force-pushed the orthoview_vlim branch from 5aab5f9 to 0cd1411 Compare May 25, 2019 23:22

effigies closed this Mar 3, 2022

ENH: Add manual value limits to OrthoSlicer3D/SpatialImages.orthoview #491

ENH: Add manual value limits to OrthoSlicer3D/SpatialImages.orthoview #491

Uh oh!

Conversation

effigies commented Oct 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

effigies commented Oct 16, 2016

Uh oh!

coveralls commented Oct 16, 2016

Uh oh!

codecov-io commented Oct 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

matthew-brett commented Oct 18, 2016

Uh oh!

effigies commented Oct 19, 2016

Uh oh!

coveralls commented Oct 19, 2016

Uh oh!

matthew-brett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

effigies commented Oct 19, 2016

Uh oh!

coveralls commented Oct 19, 2016

Uh oh!

coveralls commented Oct 19, 2016

Uh oh!

coveralls commented Oct 20, 2016

Uh oh!

matthew-brett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthew-brett commented Nov 26, 2016

Uh oh!

matthew-brett commented Jan 12, 2017

Uh oh!

effigies commented Jan 12, 2017

Uh oh!

matthew-brett commented Feb 26, 2017

Uh oh!

effigies commented Feb 28, 2017

Uh oh!

matthew-brett commented Mar 3, 2017

Uh oh!

effigies commented Mar 6, 2017

Uh oh!

coveralls commented Jul 7, 2017

Uh oh!

nibotmi commented Oct 6, 2017

Uh oh!

nibotmi commented Jun 2, 2018

Uh oh!

nibotmi commented Nov 29, 2018

Uh oh!

Uh oh!

effigies commented Oct 16, 2016 •

edited

Loading

codecov-io commented Oct 16, 2016 •

edited

Loading