-
Notifications
You must be signed in to change notification settings - Fork 356
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 bike team pursuit benchmark (rewriting 1153) #1387
Conversation
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 guess the following still holds from the hold PR
-
Women and MenTeamPursuit classes look like they could be factorized further, can you point out what differs? For now I am seeing a lot of redundant code, we that much redundancy I may stop providing support for this test case if I need to refactorize some part of ExperimentFunction or the parametrization (no plan yet), because repetitions make maintenance more complicated. -> let's merge as is I guess, but I'm concerned that this can eventually serve as example to other benchmarks which will be far from the quality standard of the others
-
Can you a add a dummy test just to make sure the function works, independently of the benchmark, that could be something as simple as that:
Co-authored-by: Jérémy Rapin <[email protected]>
I factorized quite a bit and added a simple test. |
@ryankroon could we have your opinion here ? |
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.
Please rename the main public facing class to use pep8 convention (capitalized name).
All the others should use the same convention as well in a perfect world... but at the very very least the main one must absolutely.
When that's done, feel free to merge
Co-authored-by: Jérémy Rapin <[email protected]>
Co-authored-by: Jérémy Rapin <[email protected]>
Co-authored-by: Jérémy Rapin <[email protected]>
Co-authored-by: Jérémy Rapin <[email protected]>
Hi @teytaud and @jrapin, |
Types of changes
Motivation and Context / Related issue
See #1153
How Has This Been Tested (if it applies)
Checklist