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

Laplace performance test #1908

Merged
merged 14 commits into from
Feb 4, 2020
Merged

Laplace performance test #1908

merged 14 commits into from
Feb 4, 2020

Conversation

JosephThomasParker
Copy link
Contributor

Adds a simple performance benchmark for timing Laplacian inversion.

@d7919
Copy link
Member

d7919 commented Jan 30, 2020

Looks good to me. Will have to be careful about cases where we might want to provide an initial guess (iterative solvers) as in real cases we would potentially have a decent (or not) guess, whilst here we'd always have the same guess (or a perfect guess if we reused the previous result).

examples/performance/laplace/data/BOUT.inp Outdated Show resolved Hide resolved

MZ = 32 # Z size

grid = "../../../tests/integrated/test-laplace/test_laplace.grd.nc"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the information in these grid files is pretty trivial. Maybe better to put the variables into [mesh] section here? Might even just be defaults dx=dy=dz=g11=g22=g33=1, g12=g13=g23=0 at a guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked the gridfile, and really all that's in it that's non-default is nx and ny, so can replace with

nx = 12
ny = 16

in the [mesh] section. Actually for a performance test, probably want to increase those numbers a bit... I guess ny does not really matter, effectively it just multiplies the number of iterations and averages over a few slightly different inputs, but a nx and mz in the range 100-200 is probably more representative.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is following the style of the other performance tests/examples where a scan over grid size is one of the use cases (to look at weak scaling as well as strong scaling etc.). In the other performance cases I think this is included as a part of the script used to run the scan.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks both. I was following the iterator/make_scaling_example.sh style in runtest, but I stripped out the grid size scan. I can put that back in if people prefer?

(Also, I went with small numbers just to have something that runs in a few seconds on O(1) cores by default.)

Copy link
Member

Choose a reason for hiding this comment

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

If you wanted to be fancy you could add some arguments to the script so you can ask for a grid size scan, proc scan or both.

examples/performance/laplace/data/BOUT.inp Outdated Show resolved Hide resolved
examples/performance/laplace/laplace.cxx Outdated Show resolved Hide resolved
@d7919
Copy link
Member

d7919 commented Feb 4, 2020

Should we also backport this? Might be handy to see how the performance has changed between 4.4 and 5.0

@JosephThomasParker
Copy link
Contributor Author

Should we also backport this? Might be handy to see how the performance has changed between 4.4 and 5.0

Yep, will do.

@d7919 d7919 added the backport candidiate Does not break backward compatibility, so can be back-ported to v4.4 label Feb 4, 2020
@ZedThree ZedThree merged commit 6782d5a into next Feb 4, 2020
@ZedThree ZedThree deleted the feature/laplace-performance-test branch February 4, 2020 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport candidiate Does not break backward compatibility, so can be back-ported to v4.4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants