-
Notifications
You must be signed in to change notification settings - Fork 3
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
Splines improve benchmark #496
Draft
blegouix
wants to merge
36
commits into
main
Choose a base branch
from
splines-improve-benchmark
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 8 commits
Commits
Show all changes
36 commits
Select commit
Hold shift + click to select a range
169efae
align reference preconditioner size on default
blegouix 20ce63c
wip
blegouix c498306
wip
blegouix cce3cfd
wip
blegouix 8cc0125
sweep on splines degree
blegouix 76c146c
wip
blegouix b0e0dde
non-uniform/uniform
blegouix 2d217c7
wip
blegouix 28fe503
sweep on exec_space
blegouix c741315
wip
blegouix 5dba374
wip
blegouix 93ffde4
wip
blegouix 9310a70
wip
blegouix a4d64b6
wip
blegouix 0692da1
restructurate plotter
blegouix 21c4516
add degree and uniformity
blegouix d6eaac3
finish plotter restructurateion
blegouix 3232c76
minor
blegouix d029688
wip
blegouix d4fa428
wip
blegouix 2a4977f
misc
blegouix 0b7baa4
_suffix the file names
blegouix 67155d6
ifs in plotter
blegouix fd03143
minor
blegouix 7c897a6
backend suffix
blegouix d11ae29
minor
blegouix a1d4475
minor
blegouix 54e71cd
minor
blegouix 5b0bb19
fix
blegouix bc6510d
fix
blegouix 9065d88
Merge branch 'main' into splines-improve-benchmark
blegouix 386ad4f
Merge branch 'main' into splines-improve-benchmark
blegouix 0770af3
minor
blegouix 26561fe
Merge branch 'splines-improve-benchmark' of github.com:Maison-de-la-S…
blegouix 5527831
lin scale precond
blegouix b4f969b
Update benchmarks/splines.cpp
blegouix File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@tpadioleau any opinion about this ? It is hacky but helps a lot to make the benchmarks flexible. I did not find yet any way to write it as "nested loops", maybe there is one still.
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 can imagine an improvement of TypeSeq with something like:
And upon this a nested parameter pack expansions on
TypeSeq<Kokkos::DefaultHostExecutionSpace, Kokkos::DefaultExecutionSpace>
,TypeSeq<std::false_type, std::true_type>
andTypeSeq<3, 4, 5>
should be feasible, but it may be a bit too complicated.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.
Don't you think you need different tests for Ginkgo and Lapack ? They have different parameters ?
cols_per_chunk_ref
andpreconditionner_max_block_size_ref
do not make sense for Lapack right ?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.
If you want to select a benchmark based on a runtime parameter could we use a string and store the different benchmarks in a
std::map<std::string, benchmark> my_benchmarks
? you would just callmy_benchmarks.at("host")
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.
Atm the choice of Backend still has to be manually changed by editing the file.
cols_per_chunk_ref
andpreconditionner_max_block_size_ref
do not make sense for Lapack indeed, and this is weird to have them passed as optional arguments of SplineBuilder. But this is the current implementation, we could do something better with a kwArgs struct (similar to what we have in the FFT) but this requires more developments.About the benchmark, the current way to use it is to comment/uncomment the benchmarks we want to run so we just have to not run the preconditioner or cols_per_chunk benchmark with Lapack.
Making a more interfaced benchmark system would be great but requires development. And I think it would not impact the way benchmarks are performed (this would be a software overlay build upon it). Maybe gtest already provides something to do it.
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.
(my question was more about instanciating all the possible variations of the class we would like to benchmark, combining explicitly compile-time parameters like ExecutionSpace, Degree and Uniformity)
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.
Ok I think I get the problem, would this google benchmark feature https://github.com/google/benchmark/blob/main/docs/user_guide.md#templated-benchmarks-that-take-arguments help you ?
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 don't think so because I think it does not allow to "sweep" on those template parameters (i.e. degree={3,4,5}) so it would be at least less conveniant than my solution (if I understand well the capabilities of this feature).