BUGFIX: @GaelVaroquaux's comment on PR 280 #291

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

dohmatob commented Sep 2, 2013

slicerparameter was not being passed to get_cut_coordsfunction. As a consequence only the default slicer value 'z' was begin used back-end.

Contributor

bthirion commented Sep 3, 2013

Great, thanks. Do you see any way to test that ?

Member

GaelVaroquaux commented Sep 3, 2013

Do you see any way to test that ?

Indeed, the function should be tested on synthetic data to make sure that
it returns different results that make sens in each case.

Contributor

dohmatob commented Sep 4, 2013

Yes, I'll do this asap.

Owner

matthew-brett commented Sep 25, 2013

Any progress on testing?

Member

GaelVaroquaux commented Sep 26, 2013

@dohmatob : @matthew-brett is right, this PR is lingering. Do you think that you could add tests to it?

Contributor

dohmatob commented Sep 26, 2013

Yes, I should have committed some tests and an improvement of my original
heuristic by the end of the day (GMT).

On Thu, Sep 26, 2013 at 9:01 AM, Gael Varoquaux notifications@github.comwrote:

@dohmatob https://github.com/dohmatob : @matthew-bretthttps://github.com/matthew-brettis right, this PR is lingering. Do you think that you could add tests to it?


Reply to this email directly or view it on GitHubhttps://github.com/nipy/nipy/pull/291#issuecomment-25147359
.

DED

Contributor

dohmatob commented Sep 27, 2013

OK, just added the tests.
@GaelVaroquaux : computed cut_coords are now maximally separated, and we capture all 'interesting' planes in only 5 cuts.

Contributor

dohmatob commented Sep 27, 2013

Much better with improved heuristic (maximally separated cut_coords, to sweep the whole brain with fewer cuts)

Owner

matthew-brett commented Sep 30, 2013

@GaelVaroquaux - any comments here?

slicer='z',
black_bg=True, # looks much better thus
figure=10,
- threshold=2.5)
+ threshold=2.3)
@bthirion

bthirion Sep 30, 2013

Contributor

Why 2.3 (just curious)?

@GaelVaroquaux

GaelVaroquaux Oct 9, 2013

Member

Actually, I think that it should be a percentile, and more a percentile of the abs of the map. The reason that you want a percentile is that the maps could be anything. You don't know in what unit they are.

I have a fast percentile computing code in the viz_tools code base for this purpose.

@bthirion

bthirion Oct 9, 2013

Contributor

I agree that percentiles are very useful in general, for instance when
you're dealing with independent components maps etc.
However, I tend to disagree when it's about a z map that comes from a
glm: the common use in the community is to threshold according a given
type I error control. The defaults thresholds adpted e.g. in SPM are

  • p < 0.001 uncorrected (z >3.09).
  • or p < 0.05 corrected (z>4.8 approximately if you use 3mm voxels).
    I think that these are reasonable rules -- as long as we're dealing
    with a z map.

Sorry for bringing such a mess in a quite unrelated PR.

@dohmatob

dohmatob Oct 9, 2013

Contributor

Sorry all, I've off-line for a while. OK, @bthirion: In the demo, I
pseudo-randomly tweaked the threshold value from 2.5 to 2.3, loosely saying
to myself, "after all, one number is not as good as another". You're right,
'low' thresholds will only lead to noisy figures. Here, the threshold is
for a z statistic map indeed.

Altogether, this is just a demo, and the user/caller will always be
responsible for deciding a value, --and a meaning-- for the threshold
parameter. At the moment, no assumption what soever is made about the
actual meaning of this threshold, beyond the natural understanding that
"image/map values (absolute ?) less than threshold should be taken as zero".

On Wed, Oct 9, 2013 at 8:04 PM, bthirion notifications@github.com wrote:

In examples/labs/need_data/localizer_glm_ar.py:

          slicer='z',
          black_bg=True,  # looks much better thus
          figure=10,
  •         threshold=2.5)
    
  •         threshold=2.3)
    

I agree that percentiles are very useful in general, for instance when
you're dealing with independent components maps etc. However, I tend to
disagree when it's about a z map that comes from a glm: the common use in
the community is to threshold according a given type I error control. The
defaults thresholds adpted e.g. in SPM are - p < 0.001 uncorrected (z

3.09). - or p < 0.05 corrected (z>4.8 approximately if you use 3mm
voxels). I think that these are reasonable rules -- as long as we're
dealing with a z map. Sorry for bringing such a mess in a quite unrelated
PR.


Reply to this email directly or view it on GitHubhttps://github.com/nipy/nipy/pull/291/files#r6864257
.

DED

@GaelVaroquaux

GaelVaroquaux Oct 9, 2013

Member

However, I tend to disagree when it's about a z map that comes from a
glm:

Point taken. I agree with you in this specific setting.

G

Contributor

schwarty commented Oct 8, 2013

Any reason not to close the PR?

Owner

matthew-brett commented Oct 8, 2013

I was waiting for feedback from Gael. Also - any comments on Bertrand's question?

Contributor

schwarty commented Oct 8, 2013

I'll ping @GaelVaroquaux for the feeback. Regarding Bertand's question, my guess is that @dohmatob took roughly 2 standard deviations from the mean on those data. But then it's only an example so maybe it just looks better.

Contributor

bthirion commented Oct 9, 2013

All right, but a threshold of 3 is a better practice (and actually yields nicer pictures). Sorry for nit-picking.
But, please, merge.

Owner

matthew-brett commented Oct 9, 2013

Gael - you are happy for this one to be merged?

nipy/labs/viz_tools/activation_maps.py
@@ -152,8 +155,11 @@ def plot_map(map, affine, cut_coords=None, anat=None, anat_affine=None,
warnings.warn('Mayavi > 3.x not installed, plotting only 2D')
do3d = False
- if cut_coords is None and slicer in 'xyz':
- cut_coords = get_cut_coords(map)
+ if (cut_coords is None or len(np.shape(cut_coords)) == 0
@GaelVaroquaux

GaelVaroquaux Oct 10, 2013

Member

Rather than 'len(np.shape(cut_coords)) == 0', I think that I would prefer 'isinstance(cut_coords, numbers.Number)'.

@GaelVaroquaux

GaelVaroquaux Apr 3, 2014

Member

@dohmatob : could you please address this comment: I would prefer 'isinstance(cut_coords, numbers.Number)'.

nipy/labs/viz_tools/coord_tools.py
+ """
+
+ # k < 2 is senseless
+ k = max(k, 2)
@GaelVaroquaux

GaelVaroquaux Oct 10, 2013

Member

You probably want to emit a warning here, so that such casting doesn't go unnoticed.

@GaelVaroquaux

GaelVaroquaux Apr 3, 2014

Member

@dohmatob : a warning here please.

nipy/labs/viz_tools/coord_tools.py
- delta_axis: int, optional (default 3)
- spacing between cuts
+
+ cut_coords: int > 1, optional (default None)
@GaelVaroquaux

GaelVaroquaux Oct 10, 2013

Member

I think that I prefer the name that you had originally, ie 'n_cuts'. It is more explicit.

Member

GaelVaroquaux commented Oct 10, 2013

Good job! I like your heuristic. I think that it should work fairly well in practice.

I made two cosmetic comments, that I'd like you to address, and after that, I am +1 for merge. Thanks a lot

Owner

matthew-brett commented Oct 21, 2013

@dohmatob - can you address Gael's comments? Then we can merge. Thanks again for the work.

Owner

matthew-brett commented Oct 29, 2013

@dohmatob - can you address Gael's comments? If not - let me know?

Contributor

dohmatob commented Oct 29, 2013

OK, let me look at this now.

Contributor

dohmatob commented Oct 29, 2013

OK, @schwarty is kind enough to let me try the heuristic on his bail of activation maps. I should have stronger QA conclusions with more data. I should address Gael's comments too by tomorrow.

Contributor

bthirion commented Nov 25, 2013

We should merge that one. Is everybody happy with it as is ?

Member

GaelVaroquaux commented Nov 27, 2013

Well, non of my latest comments have been addressed. @dohmatob ?

Owner

matthew-brett commented Mar 26, 2014

@dohmatob - would you mind addressing Gael's comments on this one?

Contributor

dohmatob commented Mar 28, 2014

Hi @matthew-brett . OK let me address this now.

Contributor

dohmatob commented Apr 3, 2014

Update: comments addressed. commit 6311e94.

Member

GaelVaroquaux commented Apr 3, 2014

OK, it took me a while to realize what was going on here. @dohmatob has pushed his modifications to a new branch: https://github.com/dohmatob/nipy/tree/auto-cut-coords

It makes the review work a bit hard :(

@GaelVaroquaux GaelVaroquaux referenced this pull request Apr 3, 2014

Merged

Auto cut coords #314

Coverage Status

Changes Unknown when pulling f006e3d on dohmatob:auto-get-coords into * on nipy:master*.

Wait, I am lost. You have merged 'auto-get-coords' in master? What I asked you to do was to rebase 'auto-get-coords' onto master.

Right now, we don't have a PR that we can review. We need a PR with your branch rebased on master.

In addition, in the diff that I see up there, there is a 'find_tubo_cut_coords' name which looks really wrong to me.

Owner

matthew-brett commented Apr 10, 2014

Am I right that this one is superseded by #314 and can be closed?

Member

GaelVaroquaux commented Apr 10, 2014

Yes. I am closing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment