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

BF: EuDX odf_vertices param has no default value #315

Merged
merged 4 commits into from Jan 14, 2014

Conversation

Projects
None yet
4 participants
@Garyfallidis
Member

Garyfallidis commented Jan 14, 2014

The sphere used in the reconstruction step for the odf needs to be the same as the sphere used for EuDX tracking. And having a default parameter in EuDX was misleading. Now removed. Thanks to Sam for pointing out the problem.

seeds=10000,
odf_vertices=None,
seeds,
odf_vertices,

This comment has been minimized.

@stefanv

stefanv Jan 14, 2014

Contributor

Why does this not accept a Sphere instead?

This comment has been minimized.

@Garyfallidis

Garyfallidis Jan 14, 2014

Member

It could accept a Sphere. But the idea for more than 2 years now was not to work on enhancing or changing EuDX but design a new tracking API that would replace EuDX. Unfortunately, we still don't have that apart from the work that Bago did.

I personally think that it would be nicer to have a sphere as an option. But it will brake backwards compatibility for a lot of people as EuDX has been around from the early days of Dipy.

I am happy with anything that the rest decide.

This comment has been minimized.

@stefanv

stefanv Jan 14, 2014

Contributor

Best of both worlds: accept an object -- if it is an array, assume vertices, if it is a Sphere, use as-is.

This comment has been minimized.

@Garyfallidis

Garyfallidis Jan 14, 2014

Member

Sure, I will have this ready shortly.

This comment has been minimized.

@MrBago

MrBago Jan 14, 2014

Contributor

sorry guys I have to disagree here. Are we really adding much by allowing f(a, b, sphere) and f(a, b, sphere.vertices). I somewhat prefer the second, but I really don't like accepting both. Why add a try/except block or instance checking in the code all so the user does not have to type out sphere.vertices?

This comment has been minimized.

@stefanv

stefanv Jan 14, 2014

Contributor

I don't feel too strongly about this. I was thinking that perhaps the user may have his vertices as (theta, phi) coordinates, or un-normalized direction vectors, and in that case working via Sphere is a bit more safe and convenient. But since I've never used EuDX, I trust your judgement on this more than my own.

This comment has been minimized.

@Garyfallidis

Garyfallidis Jan 14, 2014

Member

Okay, it seems that there is a disagreement. So, should we then leave this discussion for a later PR? We want to release 0.7.1 tomorow if possible and it would be good to have all bug fixes included.

This comment has been minimized.

@MrBago

MrBago Jan 14, 2014

Contributor

We should definably not change the API of a major function in a bug-fix release, so this discussion would be for .8 in any case.

This comment has been minimized.

@stefanv

stefanv Jan 14, 2014

Contributor

Agreed

This comment has been minimized.

@arokem

arokem Jan 14, 2014

Member

+1 to leaving as is for now, for backwards compatibility and revisiting
after 0.7.1

On Tue, Jan 14, 2014 at 12:01 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

In dipy/tracking/eudx.py:

@@ -45,8 +45,8 @@ class EuDX(object):
'''

 def __init__(self, a, ind,
  •             seeds=10000,
    
  •             odf_vertices=None,
    
  •             seeds,
    
  •             odf_vertices,
    

Okay, it seems that there is a disagreement. So, should we then leave this
discussion for a later PR? We want to release 0.7.1 tomorow if possible and
it would be good to have all bug fixes included.


Reply to this email directly or view it on GitHubhttps://github.com//pull/315/files#r8873751
.

@arokem

This comment has been minimized.

arokem commented on dipy/tracking/tests/test_propagation.py in 1e6b32b Jan 14, 2014

Wouldn't you rather use the smaller (362) symmetric sphere here and in other examples/tests? It'll probably make the example building and testing run faster.

This comment has been minimized.

Owner

Garyfallidis replied Jan 14, 2014

Why do you say that? This test is fast! In other cases for example in reconstruction having a better sphere (with more points) helps on the test accuracy and validation.

This comment has been minimized.

arokem replied Jan 14, 2014

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jan 14, 2014

Has anyone seen travis working after the last commit? It seems that it was not triggered or that its execution is delayed. Any ideas?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jan 14, 2014

Aaah! Okay it started now :)

@@ -25,6 +25,7 @@
eu = EuDX(csapeaks.gfa,
csapeaks.peak_indices[..., 0],
seeds=10000,

This comment has been minimized.

@arokem

arokem Jan 14, 2014

Member

This is no longer a kwarg, so maybe simply:
eu = EuDX(csapeaks.gfa,
csapeaks.peak_indices[..., 0],
10000,
etc...)

This comment has been minimized.

@Garyfallidis

Garyfallidis Jan 14, 2014

Member

As this is a tutorial I want the users to start learning the tricky parameters of the functions that they use. I use also the seeds in this text so it helps in the explanation.

This comment has been minimized.

@arokem

arokem Jan 14, 2014

Member

Gotcha. No problem. Merging...

On Tue, Jan 14, 2014 at 12:53 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

In doc/examples/tracking_eudx_odf.py:

@@ -25,6 +25,7 @@

eu = EuDX(csapeaks.gfa,
csapeaks.peak_indices[..., 0],

  •      seeds=10000,
    

As this is a tutorial I want the users to start learning the tricky
parameters of the functions that they use. I use also the seeds in this
text so it helps in the explanation.


Reply to this email directly or view it on GitHubhttps://github.com//pull/315/files#r8875528
.

@@ -80,6 +81,7 @@
eu = EuDX(csapeaks.peak_values,
csapeaks.peak_indices,
seeds=10000,

This comment has been minimized.

@arokem

arokem Jan 14, 2014

Member

same here as above

arokem added a commit that referenced this pull request Jan 14, 2014

Merge pull request #315 from Garyfallidis/eudx_sphere_param
BF: EuDX odf_vertices param has no default value

@arokem arokem merged commit 314b343 into nipy:master Jan 14, 2014

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment