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

NF: Find the closest vertex on a sphere for an input vector. #483

Merged
merged 4 commits into from Dec 2, 2014

Conversation

Projects
None yet
4 participants
@arokem
Member

arokem commented Nov 26, 2014

This is useful for both #460 and #419 so needs to take precedence. It's not quite done yet, though. Will hopefully have a chance to finish this later today.

vertex (in angle).
"""
ang = np.arccos(np.dot(self.vertices, xyz))
ang = np.min(np.vstack([ang, np.pi - ang]), 0)

This comment has been minimized.

@MrBago

MrBago Nov 26, 2014

Contributor

I've been using something like the following for this:

cos_sim = abs(np.dot(self.vertices, xyz))
return np.argmax(cos_sim)

If this is being done repeatedly it might be worth building a KDtree for it.

def test_sphere_find_closest():
sphere1 = unit_octahedron.subdivide(4)
for ii in range(sphere1.vertices.shape[0]):
nt.assert_equal(sphere1.find_closest(sphere1.vertices[ii]), ii)

This comment has been minimized.

@MrBago

MrBago Nov 26, 2014

Contributor

I noticed that this test is failing because this sphere has "mirror" points. For every point X on the octahedron, there is also a point -X. We can easily get this test to pass by using something like sphere1 = hemi_icosahedron.subdivide(3), but that got me thinking. Should this method be defined only on the HemiSphere? The docs for hemisphere say "A HemiSphere is similar to a Sphere but it takes antipodal symmetry into account." We could also define different versions of this method for Sphere and HemiSphere. Thoughts?

The index into the Sphere.vertices array that gives the closest
vertex (in angle).
"""
ang = np.arccos(np.dot(self.vertices, xyz))

This comment has been minimized.

@samuelstjean

samuelstjean Nov 26, 2014

Contributor

I also have a similar function like that, and by experience not using real unit vectors (like bvecs for example, which are almost unit vector by default) let to instability in the result.

Why not make it trulty general by dividing by the l2 norm and be able to pass any vector as an added bonus? I also clip the result between -1 and 1 because of imprecision in the division step.

This comment has been minimized.

@arokem

arokem Nov 26, 2014

Member

Thanks for the comments and suggestions.

How about the following solution? Have each of these classes have it's own
(just very slightly different) implementation of this function, taking or
not taking the absolute value, as needed.

I have also added a test to demonstrate that scaling by l2norm is not
necessary, as long as you are not interested in the angle, but rather just
in finding the index of the minimal.

On Wed, Nov 26, 2014 at 12:22 PM, Samuel St-Jean notifications@github.com
wrote:

In dipy/core/sphere.py:

  • def find_closest(self, xyz):
  •    """
    
  •    Find the index of the vertex in the Sphere closest to the input vector
    
  •    Parameters
    

  •    xyz : array-like, 3 elements
    
  •        A unit vector
    
  •    Return
    

  •    idx : int
    
  •        The index into the Sphere.vertices array that gives the closest
    
  •        vertex (in angle).
    
  •    """
    
  •    ang = np.arccos(np.dot(self.vertices, xyz))
    

I also have a similar function like that, and by experience not using real
unit vectors (like bvecs for example, which are almost unit vector by
default) let to instability in the result.

Why not make it trulty general by dividing by the l2 norm and be able to
pass any vector as an added bonus? I also clip the result between -1 and 1
because of imprecision in the division step.


Reply to this email directly or view it on GitHub
https://github.com/nipy/dipy/pull/483/files#r20961761.

This comment has been minimized.

@MrBago

MrBago Nov 27, 2014

Contributor

Looks good to me! +1

In dipy/core/sphere.py:

  • def find_closest(self, xyz):
  •    """
    
  •    Find the index of the vertex in the Sphere closest to the input vector
    
  •    Parameters
    

  •    xyz : array-like, 3 elements
    
  •        A unit vector
    
  •    Return
    

  •    idx : int
    
  •        The index into the Sphere.vertices array that gives the closest
    
  •        vertex (in angle).
    
  •    """
    
  •    ang = np.arccos(np.dot(self.vertices, xyz))
    

Thanks for the comments and suggestions. How about the following solution?
Have each of these classes have it's own (just very slightly different)
implementation of this function, taking or not taking the absolute value,
as needed. I have also added a test to demonstrate that scaling by l2norm
is not necessary, as long as you are not interested in the angle, but
rather just in finding the index of the minimal.
… <#149ee0c9a5adaa44_>
On Wed, Nov 26, 2014 at 12:22 PM, Samuel St-Jean notifications@github.com
wrote: In dipy/core/sphere.py: > + def find_closest(self, xyz): > + """ > +
Find the index of the vertex in the Sphere closest to the input vector > +

  • Parameters > + ---------- > + xyz : array-like, 3 elements > + A unit
    vector > + > + Return > + ------ > + idx : int > + The index into the
    Sphere.vertices array that gives the closest > + vertex (in angle). > + """
  • ang = np.arccos(np.dot(self.vertices, xyz)) I also have a similar
    function like that, and by experience not using real unit vectors (like
    bvecs for example, which are almost unit vector by default) let to
    instability in the result. Why not make it trulty general by dividing by
    the l2 norm and be able to pass any vector as an added bonus? I also clip
    the result between -1 and 1 because of imprecision in the division step. —
    Reply to this email directly or view it on GitHub <
    https://github.com/nipy/dipy/pull/483/files#r20961761>.


Reply to this email directly or view it on GitHub
https://github.com/nipy/dipy/pull/483/files#r20965750.

@arokem arokem changed the title from WIP: Find the closest vertex on a sphere for an input vector. to NF: Find the closest vertex on a sphere for an input vector. Nov 26, 2014

@arokem

This comment has been minimized.

Member

arokem commented Nov 26, 2014

Thanks for the comments and suggestions.

How about the following solution? Have each of these classes have it's own (just very slightly different) implementation of this function, taking or not taking the absolute value, as needed.

I have also added a test to demonstrate that scaling by l2norm is not necessary, as long as you are not interested in the angle, but rather just in finding the index of the minimal.

@arokem arokem force-pushed the arokem:find_closest branch from 0f180ae to 0410bf5 Dec 2, 2014

@arokem

This comment has been minimized.

Member

arokem commented Dec 2, 2014

Just rebased on master. Could someone take a look and merge this? It's a no-brainer (if I may say so myself).

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 2, 2014

Will have a look first thing tomorrow morning. Sorry for the delay.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 2, 2014

Hi @arokem. The tests are a bit too tight because you only test finding an already existing point to itself. I would like to see in the future a test showing that if you add a new point that is equal to old point + tiny random shift you will still get the same point. But because this is a trivial problem and this PR is needed for other more complex PRs I believe is pointless to wait right now for extra testing on this and will go ahead and merge. Inf+!!!

Garyfallidis added a commit that referenced this pull request Dec 2, 2014

Merge pull request #483 from arokem/find_closest
NF: Find the closest vertex on a sphere for an input vector.

@Garyfallidis Garyfallidis merged commit 0fbe1ee into nipy:master Dec 2, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment