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

RF: Replaces 1997 definitions of tensor geometric params with 1999 definitions. #1220

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@arokem
Copy link
Member

arokem commented Apr 13, 2017

@RafaelNH just pointed out to me that Westin et al. changed their definition of planarity, linearity and sphericity in a paper that came out in MICCAI in 1999, and that some of the subsequent literature used these definitions, rather than the ones we have been using.

Compare this paper:

http://lmi.bwh.harvard.edu/papers/pdfs/1999/westinMICCAI99.pdf

Instead of this paper:

http://lmi.bwh.harvard.edu/papers/papers/westinISMRM97.html

The main feature of the new definitions is that the three measures sum to 1 in every voxel.

It seems that this is the version that people should use, but this is a change from the previous definitions in our software, and users will get different results with this change. I am not sure how to handle that. Any thoughts?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 13, 2017

Coverage Status

Coverage increased (+0.0004%) to 88.415% when pulling b9c92e1 on arokem:westin-1999 into bdee770 on nipy:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 13, 2017

Codecov Report

Merging #1220 into master will decrease coverage by <.01%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1220      +/-   ##
=========================================
- Coverage   85.91%   85.9%   -0.01%     
=========================================
  Files         221     221              
  Lines       27247   27266      +19     
  Branches     2782    2788       +6     
=========================================
+ Hits        23408   23424      +16     
+ Misses       3154    3152       -2     
- Partials      685     690       +5
Impacted Files Coverage Δ
dipy/reconst/tests/test_dti.py 99.61% <100%> (ø) ⬆️
dipy/reconst/dti.py 95.94% <82.35%> (-0.58%) ⬇️
dipy/utils/six.py 45.71% <0%> (ø) ⬆️

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 0297078...8d45577. Read the comment docs.

@etpeterson

This comment has been minimized.

Copy link
Contributor

etpeterson commented Apr 13, 2017

Nothing revolutionary in this suggestion, but I'd say support both and default to the more common one, I guess the 99 version. Probably do the typical slow rollout (implement as non-default, warn it will change, change to default and warn it changed, remove warning) over a year or so.

Also, it looks like he claims both versions sum to 1, though I haven't thought that through.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Apr 13, 2017

Thanks for the suggestion @etpeterson! I have now implemented a version key-word argument. The default behavior entails no change from the current master, but users can now select to use the "1999" version.

And to address this issue: as I discovered by running this test on both: no, the older version of these does not sum to 1. :shrug:

arokem added some commits Apr 13, 2017

RF: Allow selection of version of geometric scalars.
Use key-word argument "version". For the OO interface, the default ("1997")
is used. We cannot allow selection on the OO interface, because it depends
on the autoattr decorator, so users will have to do it on their own (from
the evals attribute), if they need this.

@arokem arokem force-pushed the arokem:westin-1999 branch from 098e83e to 8d45577 Apr 13, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 13, 2017

Coverage Status

Coverage increased (+0.008%) to 88.423% when pulling 8d45577 on arokem:westin-1999 into 0297078 on nipy:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 13, 2017

Coverage Status

Coverage increased (+0.008%) to 88.423% when pulling 8d45577 on arokem:westin-1999 into 0297078 on nipy:master.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Apr 16, 2017

Can we e-mail Westin about this? It would be great to have his viewpoint.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Apr 16, 2017

My guess is that their position would be that the latter paper supersedes the earlier paper, but asking is a good idea. Even if that is the answer, we need to consider how we transition to using the newer metrics, or possibly continue allowing use of both formulations of this.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented May 11, 2017

I talked with Westin about this in ISMRM and he told me that he will get back to me. So, even for him it was not a direct choice for 97 or 99. He said he had a personal preference for 97 but he needs to think about it.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented May 11, 2017

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Dec 11, 2018

Hi @arokem, did you get a feedback from Westin?

What is the way to go for this PR?

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Dec 11, 2018

When I spoke with Westin some time ago he told me that he is fine with what we have. We can close this PR.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 11, 2018

@skoudoro skoudoro closed this Dec 11, 2018

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Dec 11, 2018

ok, closing

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