-
Notifications
You must be signed in to change notification settings - Fork 38
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
Touchup adjoint test #214
Open
samhatfield
wants to merge
29
commits into
develop
Choose a base branch
from
touchup_adjoint_test
base: develop
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.
Open
Touchup adjoint test #214
Conversation
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
We don't need performance measurements in a correctness test.
The perturbations were only applied to the first NLEVG fields, but these fields were all zero (vorticity and divergence are ordered first and they were always zero in the test).
There was no difference between repetitions so this was pointless.
With the previous value, 100, the test failed in the Github CI suite for clang-12/Release and gnu-10/Debug.
0b6df4f
to
5127f6e
Compare
5127f6e
to
3119da6
Compare
76e782d
to
4b434be
Compare
Some compilers were failing in the NPROC=2 case.
4b434be
to
8ad9285
Compare
2ad5794
to
2d9848e
Compare
2d9848e
to
6646215
Compare
6646215
to
66a60b0
Compare
They weren't actually used for anything.
21c4557
to
5a853a1
Compare
Ready for review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
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.
The existing adjoint test program seems to check the following identity:
Where$\delta$ is an arbitrary perturbation. $X$ and $Y$ are both spectral space variables filled with random numbers between -1 and +1.
The program has a number of issues:
NFLEVG
fields ofPGP
. But because theINV_TRANS
call that fillsPGP
includes vorticity and divergence (which are both zeroed in this test), the firstNFLEVG
fields correspond to these zero-valued vorticity values, so the multiplicative perturbation does nothing. This was probably an oversight of whoever wrote the code, but in any case I don't think a perturbation is needed, i.e. you can setThis PR aims to resolve this and make the adjoint test actually useful.
I'll keep this as a draft for now while I think of any other ways the test could be improved.