Skip to content
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

Change default sphere #590

Closed
Garyfallidis opened this issue Mar 14, 2015 · 12 comments · Fixed by #1894

Comments

@Garyfallidis
Copy link
Member

commented Mar 14, 2015

We still have the symmetric724 as the default sphere. However the repulsive724 is even better.
I am suggesting to replace the default sphere with the repulsive724.

@arokem

This comment has been minimized.

Copy link
Member

commented Mar 14, 2015

+1. While you are at it, it might be worth greping around the code to see where we use symmetric724 and should be using default_sphere instead. I bet there are a couple of places like that.

@MrBago

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2015

+1 as long as default_sphere continues to be a HemiSphere :).

@samuelstjean

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2015

I really don't get why hemisphere everywhere honestly. If you mean repulsive724 as the caruyer scheme shell (the paper one, not the website one, I've been had with that, as the HCP has been also, but that's another story), then is it much better than everything else. You could also get all the other relevants one in atthe same time.

While on the subject, his website says they are on dipy, but I looked and didn't find any reference to that anywhere in to code with a quick grep ,where is the generator located?

@MrBago

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2015

HemiSphere is subclass of Sphere which adds awareness of antipodal symmetry to the class. In the presence of antipodal symmetry, using a HemiSphere instead of a Sphere is more accurate and faster. default_sphere has been a HemiSphere for several releases now and I think it should continue to be unless there is a good reason to change it.

+1 for using repulsive724.

@samuelstjean

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2015

So does it works only if the 724 sphere can be splitted as 2x362 points, or it will make a 2x724 points sphere in that case? Doubt the 724 repulsive is symmetric, but I don't have it on hand to check.

@samuelstjean

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2015

Oh and also add a deprecation first and force it later, or else anything reconstructed with the old default sphere won't be directly comparable to the new default sphere, forcing everyone to recompute all of their peaks + tractography. Even worse if the change is silent without a notification.

@MrBago

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2015

Hemisphere takes half the points if the input contains symmetric points (every point x has a corresponding point -x) and takes all the points if every point is unique (there are no points x and y such that x == -y within a small tolerance).

This isn't the type of thing you can deprecate, but if we want to be very careful here we could probably think of something.

@samuelstjean

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2015

Well, a solution could be to print a message like "in version X, default sphere will be repulsive724 instead of symmetric724". Of course one would see that prettry much often, but you won't be able to say you didn't see it coming.

@arokem

This comment has been minimized.

Copy link
Member

commented Mar 18, 2015

When would you raise this warning? For the time being default_sphere is
just a variable in the dipy.data namespace.

On Tue, Mar 17, 2015 at 9:51 PM, Samuel St-Jean notifications@github.com
wrote:

Well, a solution could be to print a message like "in version X, default
sphere will be repulsive724 instead of symmetric724". Of course one would
see that prettry much often, but you won't be able ot say you didn't see it
coming.


Reply to this email directly or view it on GitHub
#590 (comment).

@samuelstjean

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2015

on the first import of dipy.data, it would probably require an ugly global variable or something like that. Anyway. the point is it can be changed midway without telling people about it explicitely if one wants to compare data from before and after the change safely.

@jhlegarreta

This comment has been minimized.

Copy link
Contributor

commented Dec 26, 2018

So, does this mean that we

  • Change all get_sphere('symmetric724') appearances by get_sphere('repulsion724')?
  • Change the definition of default_sphere = HemiSphere.from_sphere(get_sphere('symmetric724'))
    to default_sphere = HemiSphere.from_sphere(get_sphere('repulsion724'))?

and nothing else?

@arokem If you happen to have some idea on the best way to proceed, let me know and will give it a try.

@skoudoro skoudoro added this to the 1.0 milestone Jul 9, 2019

@Garyfallidis

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

You are correct @jhlegarreta

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.