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.

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 Green model to benchmark - follow up #40

Merged
merged 2 commits into from
Jan 3, 2025

Conversation

jpaillard
Copy link
Contributor

Following up on #39 this integrates the review from @tomMoral

Copy link
Member

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! thanks for the follow up.

Before merging, can you share a screen shot of the result on BNCI competition with one competitor like CSP-LDA?

@jpaillard
Copy link
Contributor Author

I ran this yesterday against the "Aug-Cov_reg-Tang-SVM". The time may not be so informative as it ran on CPU.
image

@bruAristimunha bruAristimunha merged commit 9c20d6d into benchopt:main Jan 3, 2025
4 checks passed
@tomMoral
Copy link
Member

The neuro-green package seems to cause some issues due to pinned dependencies.
I had to skip its install in #13 so it does not break the tests for others

@jpaillard
Copy link
Contributor Author

Could you indicate the problematic dependencies?

@tomMoral
Copy link
Member

It is not completely clear as I could not pinpoint exactly which dependency caused the issue but in #13, skipping green install solved my issues.
I will try to see if it is something else (I just added back the install) but if not, we need more investigation.
If possible to avoid having fixed dependencies (with ==), that would help a lot.

@tomMoral
Copy link
Member

Ok it seems that the error was mostly bad interactions with another error with the latest version of scikit-learn.
So it is now working with both #13 and green.
That being said, it would be great to unpin the dependencies as this can break environments easily (typically this downgrade numpy<2 while some packages might require it to be above).

@jpaillard
Copy link
Contributor Author

I tried to solve this in Roche/neuro-green#6, hope it's better now.

@tomMoral
Copy link
Member

Seems much better! thanks @jpaillard :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants