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

speedup test-invpar #2344

Merged
merged 3 commits into from
Jun 4, 2021
Merged

speedup test-invpar #2344

merged 3 commits into from
Jun 4, 2021

Conversation

dschwoerer
Copy link
Contributor

On a HPC system the precious code took ~10 minutes, now it takes ~15
seconds.
It moves the loops to C++, thus avoiding most of the expensive
initialisation of BOUT++.

Resolves #2185

On a HPC system the precious code took ~10 minutes, now it takes ~15
seconds.
It moves the loops to C++, thus avoiding most of the expensive
initialisation of BOUT++.
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

tests/integrated/test-invpar/test_invpar.cxx Outdated Show resolved Hide resolved
tests/integrated/test-invpar/test_invpar.cxx Outdated Show resolved Hide resolved
tests/integrated/test-invpar/test_invpar.cxx Outdated Show resolved Hide resolved
tests/integrated/test-invpar/test_invpar.cxx Outdated Show resolved Hide resolved
tests/integrated/test-invpar/test_invpar.cxx Outdated Show resolved Hide resolved
tests/integrated/test-invpar/test_invpar.cxx Outdated Show resolved Hide resolved
tests/integrated/test-invpar/test_invpar.cxx Outdated Show resolved Hide resolved
tests/integrated/test-invpar/test_invpar.cxx Show resolved Hide resolved
tests/integrated/test-invpar/test_invpar.cxx Outdated Show resolved Hide resolved
tests/integrated/test-invpar/test_invpar.cxx Outdated Show resolved Hide resolved
Copy link
Member

@ZedThree ZedThree left a comment

Choose a reason for hiding this comment

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

Really nice, thanks @dschwoerer ! Similar speedup on my machine, down from 7 minutes to 15 seconds 🎉

The output of the test is now like:

   1 processors....
        flags ' acoef_0=1 bcoef_0=0 ccoef_0=0 dcoef_0=0 ecoef_0=0 acoef_1=1.5 bcoef_1='2.*sin(2*y)' acoef_2=1 acoef_3=1 bcoef_3=2 acoef_4=1 ccoef_4=1.793 acoef_5=1 ccoef_5=3 bcoef_5=0 acoef_6=1 dcoef_6=3.5 acoef_7=1 ecoef_7=-1 acoef_8=1 input_field_8='ballooning(exp(-y*y)*cos(z)*gauss(x,0.2))'' ... PASSED
        flags ' acoef_0=1 bcoef_0=0 ccoef_0=0 dcoef_0=0 ecoef_0=0 acoef_1=1.5 bcoef_1='2.*sin(2*y)' acoef_2=1 acoef_3=1 bcoef_3=2 acoef_4=1 ccoef_4=1.793 acoef_5=1 ccoef_5=3 bcoef_5=0 acoef_6=1 dcoef_6=3.5 acoef_7=1 ecoef_7=-1 acoef_8=1 input_field_8='ballooning(exp(-y*y)*cos(z)*gauss(x,0.2))' mesh:ixseps1=0 mesh:ixseps2=0' ... PASSED

Might be nice to move the output of the flags from the python script to the C++ file too?

tests/integrated/test-invpar/test_invpar.cxx Outdated Show resolved Hide resolved
tests/integrated/test-invpar/test_invpar.cxx Outdated Show resolved Hide resolved
WithQuietOutput unconditionally enabeled the output again. To enable the
output properly again, it needs to know the actual state and stores
that.
Thanks @ZedThree for the suggestions.

Now the test results are printed from the C++ code.
@ZedThree
Copy link
Member

ZedThree commented Jun 4, 2021

I bet there's a fair few other tests where we loop over things in python, when we could be doing that in the C++ file. There's also lots of tests where we save fields to disk and then read them in only to do max(abs(result - expected)), when we could also be doing that in C++ and just return error from main.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

tests/integrated/test-invpar/test_invpar.cxx Show resolved Hide resolved
tests/integrated/test-invpar/test_invpar.cxx Show resolved Hide resolved
@ZedThree
Copy link
Member

ZedThree commented Jun 4, 2021

This shaves 5-10 minutes off each CI job, so we save ~1 hour of cpu-time for each PR 🎉

A really awesome PR, thanks again @dschwoerer !

@ZedThree ZedThree merged commit ebc1efb into next Jun 4, 2021
@ZedThree ZedThree deleted the test-invpar branch June 4, 2021 16:45
@ZedThree ZedThree mentioned this pull request Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants