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

Allow the use of phikon features in the benchmark script #297

Merged
merged 7 commits into from
Jan 9, 2024
Merged

Allow the use of phikon features in the benchmark script #297

merged 7 commits into from
Jan 9, 2024

Conversation

xavier-owkin
Copy link
Contributor

@xavier-owkin xavier-owkin commented Jan 4, 2024 •
edited
Loading

Remove the hardcoded 2048 dimension in order to use phikon's features in the benchmark script
phikon

@xavier-owkin xavier-owkin requested a review from jeandut as a code owner January 4, 2024 15:20
@xavier-owkin xavier-owkin requested a review from a user January 4, 2024 15:20
@xavier-owkin xavier-owkin changed the title Use phikon benchmark Allow the use of phikon features in the benchmark script Jan 4, 2024
@jeandut
Copy link
Contributor

jeandut commented Jan 4, 2024 •
edited
Loading

Can you rebase this PR @xavier-owkin ?

jeandut
jeandut reviewed Jan 5, 2024
View reviewed changes
Copy link
Contributor

@jeandut jeandut left a comment

Choose a reason for hiding this comment

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

So there is some rebasing to do as always.
Only one major comment about passing features_dimension to Baseline it needs to be done in fed_benchmark as well.

jeandut
jeandut reviewed Jan 8, 2024
View reviewed changes
Copy link
Contributor

@jeandut jeandut left a comment

Choose a reason for hiding this comment

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

Maybe we can add the results and plot as well with a _phikon tag ?

@xavier-owkin
Copy link
Contributor Author

yes, I was waiting for the results to arrive

jeandut
jeandut approved these changes Jan 9, 2024
View reviewed changes
Copy link
Contributor

@jeandut jeandut left a comment

Choose a reason for hiding this comment

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

LGTM

@xavier-owkin xavier-owkin merged commit c658e54 into main Jan 9, 2024
5 checks passed
@xavier-owkin xavier-owkin deleted the use-phikon-benchmark branch January 9, 2024 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeandut jeandut jeandut approved these changes

@maikia maikia Awaiting requested review from maikia maikia is a code owner

@tanguy-marchand tanguy-marchand Awaiting requested review from tanguy-marchand tanguy-marchand is a code owner

@regloeb regloeb Awaiting requested review from regloeb regloeb is a code owner

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

Successfully merging this pull request may close these issues.
2 participants
@xavier-owkin @jeandut