-
Notifications
You must be signed in to change notification settings - Fork 99
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
Added new tests for two consecutive adjoint runs #362
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #362 +/- ##
==========================================
+ Coverage 41.02% 41.05% +0.02%
==========================================
Files 13 13
Lines 4119 4119
==========================================
+ Hits 1690 1691 +1
+ Misses 2429 2428 -1 ☔ View full report in Codecov by Sentry. |
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.
See my comments below, I don't think we should add separate reference results for the second sensitivity analysis, am I missing anything?
tests/reg_tests/reg_test_utils.py
Outdated
CFDSolver.evalFunctionsSens(ap, funcsSens, evalFuncs=None) | ||
CFDSolver.evalFunctionsSens(ap, funcsSens, evalFuncs=None) | ||
handler.root_print("Eval Functions Sens 2:") | ||
handler.root_add_dict("Eval Functions Sens 2:", funcsSens, rtol=rtol, atol=atol) |
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.
Shouldn't we compare the second run against the values of the first run Eval Functions Sens
? I don't see the point of adding this if the point of the test is to make sure that the adjoint converges to the same result (within a tolerance)
tests/reg_tests/reg_test_utils.py
Outdated
@@ -43,7 +61,7 @@ def assert_residuals_allclose(handler, CFDSolver, ap, **kwargs): | |||
res /= totalR0 | |||
handler.root_print("Norm of residual") | |||
handler.par_add_norm("Norm of residual", res, rtol=rtol, atol=atol) | |||
|
|||
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.
This is probably causing the black and flake8 error
@@ -1,86 +1,87 @@ | |||
{ | |||
"Eval Functions Sens:": { | |||
"Adjoint States": 0.0, | |||
"Eval Functions Sens 2:": { |
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.
As I mention below, I don't think we should add this, ideally the only thing added to the .json
files is the Adjoint States
entry
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.
The added tests look good, I only think the .json
files need to be cleaned up - see my comment below
@@ -1,86 +1,87 @@ | |||
{ | |||
"Adjoint States": 0.0, |
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.
This entry should be the only one changing in the .json
files. There is no point changing the reference values for this PR, especially if the training is done on a local machine.
Also note that the black and flake8 tests are failing, please fix them. Remember that we currently use |
Purpose
In the development effort for SST, it was discovered that Tapenade was causing changes to the solutions states after an adjoint run due to a bug. To ensure that this was not a systematic problem throughout the code and to catch future instances of this issues appearing, I added two new adjoint tests. The first runs the adjoint twice back to back to ensure that the results didn't change. The second runs the adjoint twice back to back and checks to make sure that the states have not changed between the runs.
Expected time until merged
2-3 weeks
Type of change
Testing
The new feature is a new test.
Checklist
flake8
andblack
to make sure the Python code adheres to PEP-8 and is consistently formattedfprettify
or C/C++ code withclang-format
as applicable