giotto-ai / giotto-tda

A high-performance topological machine learning toolbox in Python
https://giotto-ai.github.io/gtda-docs
Other
858 stars 175 forks source link

Transpose the output of PairwiseDistance to match scikit-learn convention #420

Closed wreise closed 4 years ago

wreise commented 4 years ago

Reference issues/PRs None

Types of changes

Description Currently, the shape of a pairwise_distance.transform(X) is (X_fit.shape[0], X.shape[0]), where pairwise_distance = PairwiseDistance().fit(X_fit).

The above convention is the opposite the one in sklearn.neighbors.kNearestNeighbors. Hence, I propose to transpose the output of PairwiseDistance().transform to match that of sklearn.

Screenshots (if appropriate)

Any other comments?

Checklist

ulupo commented 4 years ago

I agree with this change, thanks for suggesting it! Another approach to this (that I might slightly prefer) would be to revise the code for _parallel_pairwise. I wonder what @gtauzin as he is the original author of this.

ulupo commented 4 years ago

@wreise may I propose the following changes which should yield a simpler approach (against my own initial suggestions of intervening on _parallel_pairwise, sorry!): https://github.com/ulupo/giotto-tda/commit/69d65859fb6c217d413dad051069d116d37517ab

ulupo commented 4 years ago

@wreise I've attempted to implement the approach I proposed in https://github.com/ulupo/giotto-tda/commit/69d65859fb6c217d413dad051069d116d37517ab, let me know what you think!

wreise commented 4 years ago

@wreise I've attempted to implement the approach I proposed in ulupo@69d6585, let me know what you think!

Thanks, @ulupo - it looks great!

ulupo commented 4 years ago

@wreise thanks. Let's wait for @gtauzin's approval!