speedup pairwise_distance #256
Conversation
|
Slight problem as logdet and wasserstein distances don't return 0 for same inputs. |
|
Probably due to numerical instability of numpy.linalg.slogdet. |
There was a problem hiding this comment.
basically I would avoid the new public functions (make them private) and I would not do any magic with "np.array_equal(X, Y)" if Y=X then user should just pass Y = None. You should now what you are doing.
Finally do not set the diag to zero. If it's 1e-15 than it's a numerical zero. Basically don't hack the numerical noise.
doc/api.rst
Outdated
| pairwise_distance_euclid | ||
| pairwise_distance_harmonic | ||
| pairwise_distance_logeuclid | ||
| pairwise_distance_riemann |
There was a problem hiding this comment.
I would not make these functions public to avoid over crowding the public namespace.
Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
|
it's better to fix LLE then
… Message ID: ***@***.***>
|
@agramfort doesn't need fixing, I remembered incorrectly. |
|
Can you see why CIs fail ? Now from an API perspective I think we should have only one public function called pairwise_distances that takes a metric parameter. It will make the API lighter.
|
it says "FileNotFoundError: [Errno 2] No such file or directory: '/tmp/sphinx-log'" which I don't really understand. I thought it was an issue on the server side.
I removed the individual functions from the api page, I thought that was enough. |
|
yes if it does not start with some _ then it's a public function. Can you
prepend some _ the function name for each
function you want to keep private? thx
… Message ID: ***@***.***>
|
Done. |
pyriemann/utils/distance.py
Outdated
| pairwise_distance | ||
| distance_harmonic | ||
| """ | ||
| if isinstance(Y, type(None)): |
There was a problem hiding this comment.
| if isinstance(Y, type(None)): | |
| if Y is None: |
pyriemann/utils/distance.py
Outdated
| between elements of X and Y. | ||
| Notes | ||
| ----- | ||
| .. versionadded:: 0.6 |
There was a problem hiding this comment.
You don't need these now that it's private
doc/whatsnew.rst
Outdated
| @@ -10,6 +10,7 @@ A catalog of new features, improvements, and bug-fixes in each release. | |||
| v0.6.dev | |||
| -------- | |||
|
|
|||
| - Speedup pairwise distance function :func:`pyriemann.utils.distance.pairwise_distance` by adding individual functions. :pr:`256` by :user:`gabelstein` | |||
There was a problem hiding this comment.
Say for what metric it's faster
There was a problem hiding this comment.
Thx @gabelstein for this nice contribution!
We can merge after #254
|
Please update the branch so the CI can turn green now
|
|
@qbarthelemy 's changes just gave me an idea. Setting the invY and logY to none when applicable speeds up the internal sklearn pairwise distance. |
|
Thx @gabelstein |
closes #224