Closed okBrian closed 1 month ago
@henryleberre can confirm but I think this might be backwards. lhs -> master and rhs -> PR.
If the variable speedup
below is > 1 then master is slower than then PR.
If the variable speedup
below is < 1 then PR is slower than master.
At least that's my read of the code...
Code:
and your code says:
cons.print(f"[bold]\t1.5x indicates [magenta]{os.path.relpath(ARG('rhs'))}[/magenta] is 1.5-times as fast as [magenta]{os.path.relpath(ARG('lhs'))}[/magenta] (so [magenta]{os.path.relpath(ARG('rhs'))}[/magenta] is faster than [magenta]{os.path.relpath(ARG('lhs'))}[/magenta]).[/bold]")
Let's consider an example:
$ ./mfc.sh bench_diff lhs rhs
where lhs
and rhs
are YAML files. By the way, "lhs" and "rhs" just stand for "left-hand side" and "right-hand side". We could maybe opt for better names.
Say that for a given example case, you have:
simulation
lhs: 1.0s
rhs: 0.5s
So, $\frac{\text{lhs}}{\text{rhs}}=\frac{1.0}{0.5}=2$. So $\frac{\text{lhs}}{\text{rhs}}$ is the speedup going from lhs
to rhs
. In other words, lhs
is what you are comparing rhs
to. This implies that the text I had written and the text @okBrian wrote are both correct, although mine was worded in a way that might be confusing. It was like "LHS is X times SLOWER than RHS" rather than "RHS is X times FASTER than LHS" (@okBrian's version).
As a sidenote, there are a lot of cons.write
calls that follow each other. These can be joined to be more readable like:
cons.write(f"""\
Line 1
Line 2
""")
There should just be a line of text above the results that says "numbers < 1 indicate the PR is faster than master, > 1 indicate PR is slower" (if that is actually the case)
PR merges are on hold until benchmark CI is working (issue #419)
@okBrian Why did you revert with 1e30c18 ?
The past few tests have either failed or have the wrong output so I'm just making sure that's not the issue.
@okBrian please sync your branch with master, which I just merged to (that's why your current CI tests are failing)
CI benchmarking is failing and it seems related to this PR
This might need another merge with master
Description
Fixed the clarity issue for the CI benchmark comparison message when comparing a PR and master.
Example:
Fixes #395
Type of change
Scope
How Has This Been Tested?
Not sure how to run the diff benchmark comparison without the github runner
Test Configuration:
Compiled on a
Ubuntu 22.04.4 using Docker Intel i7-1165G7
Checklist
docs/
)examples/
that demonstrate my new feature performing as expected. They run to completion and demonstrate "interesting physics"./mfc.sh format
before committing my codeIf your code changes any code source files (anything in
src/simulation
)To make sure the code is performing as expected on GPU devices, I have:
nvtx
ranges so that they can be identified in profiles./mfc.sh run XXXX --gpu -t simulation --nsys
, and have attached the output file (.nsys-rep
) and plain text results to this PR./mfc.sh run XXXX --gpu -t simulation --omniperf
, and have attached the output file and plain text results to this PR.