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.

Sign up for GitHub

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

Jump to bottom

Add partial_fit function to Whitening for online applications #277

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Add partial_fit function to Whitening for online applications #277

wants to merge 11 commits into from

Conversation

brentgaisford
Copy link

Add Iterative_Whitener class to pyriemman/preprocessing

@qbarthelemy
Copy link
Member

Thank you for this contribution!

However, I think there is no point in creating a new class Iterative_Whitener.
If I understand the explanations correctly, in class Whitening, it is enough to implement a partial_fit function which would update the model in an online way (see ‎Potato.partial_fit used in this example), then apply transform. Is it right?

@brentgaisford
Copy link
Author

Yes, it could be implemented that way as well if you think that's a better fit with the pattern of how other classes work. It would need to work fairly differently than the partial fit method of the potato though. If that architecture is preferred, let me know and I can submit a new pull request that implements it that way (as long as you don't mind that the partial_fit would work only with no dimension reduction and only with the euclidian metric - I don't plan to do the adaptation for the iterative method to work with other settings). With that change when running online it would require two lines instead of one (a partial_fit and then a transform), but that's no big deal.

@qbarthelemy
Copy link
Member

qbarthelemy commented Jan 18, 2024 •
edited

There is no need to open a new pull-request.
You can update this pull-request by adding the partial_fit to the Whitening class.
Thanks!

@qbarthelemy
Copy link
Member

qbarthelemy commented Jan 29, 2024 •
edited

I added partial_fit function to Whitening.
@brentgaisford, can you test this feature? Is it ok for you?

You should find your use case:

  • using fit for the initialization step;
  • using alpha equal to 1/count into partial_fit for online update;
  • and then applying transform for online whitening.

@qbarthelemy qbarthelemy changed the title Add functionality to whiten covariance matrices in online applications Add patial_fit function to Whitening for online applications Jan 29, 2024
@brentgaisford
Copy link
Author

brentgaisford commented Feb 2, 2024 via email

@gcattan
Copy link
Collaborator

gcattan commented Mar 2, 2024

Hi @brentgaisford It looks like an interesting feature :) Did you have a chance to test it?

@brentgaisford
Copy link
Author

brentgaisford commented Mar 3, 2024 via email

@gcattan
Copy link
Collaborator

gcattan commented Mar 3, 2024 via email

@brentgaisford brentgaisford changed the title Add patial_fit function to Whitening for online applications Add partial_fit function to Whitening for online applications Mar 19, 2024
@qbarthelemy
Copy link
Member

@brentgaisford , were you able to test the new partial_fit function?

@brentgaisford
Copy link
Author

I did! I had to make some changes to get it to work, but I have it running now. I want to do some more checking though to confirm the results it returns are correct - and it also seems to be slow to run, which isn’t good online. Will reply with more details next week once I can confirm results.

@brentgaisford
Copy link
Author

Ok, thanks for the patience everyone! Made some tweaks to make the new partial_fit function work seamlessly in online applications, and also made sure that the results were the same as running a fit every tick (which they are). It's about 7% slower than the original iterative_whitener class I had created, but I think that's a small enough difference that it's justified given the match with design of other functions in the library. So, from my perspective, I think this is ready to be merged! Anything else we need to address, or any questions?

@qbarthelemy
Copy link
Member

qbarthelemy commented Apr 18, 2024 •
edited

I simplified the code and re-added the dimension reduction.
@brentgaisford , can you confirm that this new implementation covers your use case?

@brentgaisford
Copy link
Author

brentgaisford commented Apr 22, 2024 via email

@gcattan
Copy link
Collaborator

gcattan commented May 7, 2024

Hi @brentgaisford did it work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qbarthelemy qbarthelemy qbarthelemy left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@brentgaisford @qbarthelemy @gcattan