-
Notifications
You must be signed in to change notification settings - Fork 164
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
Add Tangent Space Alignment #320
base: master
Are you sure you want to change the base?
Conversation
I decided to redo the TSA like the RPA, ie like a pipeline of transformers. I used prefix To be coherent, we could change the prefix of transformations in manifold, Another solution, maybe better, is to have a single class What do you think @agramfort , @sylvchev and @plcrodrigues ? Documentation of To be coherent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @qbarthelemy for this new feature on pyriemann
(and sorry for the delay for the review).
The code and implementation of TSA looks good to me. Did you use the same code as the one provided by Alexandre Bleuzé? Maybe add a comment about it on the docstring?
I must admit that I would have preferred to keep the names of the classes with TL
in caps at the beginning. And I find it a bit confusing to add Ts
and Ma
on the name of the class. Would it make sense (and doable) to always use the TLCenter
class and have an option to say whether we're doing stuff on the manifold or the tangent space? (same thing for stretch and rotate). This would allow people to easily switch between operations and even create pipelines that start on manifold and finish on tangent space.
Thanks @plcrodrigues for your feedback. It's a good idea not to duplicate classes. In the lastest version, there is a single class I complete documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to force the user to explicitly say whether he wants to do the centering (and stretching and rotation) on the manifold or the tangent space. Of course, the fit
function can detect what kind of data is given depending of its dimensions, but I find the current implementation a bit too implicit and it could be annoying to debug or explain the code later. Why not instantiate the classes saying where the transformations are done?
Also, I still prefer calling things as TLCenter
instead of TlCenter
, etc. Since it is an acronym, it makes more sense to me to keep things with capital letters.
Ok for me to keep prefix |
Deep refacto of code shared in #319.