Fix the light tests failure on MacOS - #382
Conversation
|
Job Documentation, step Sync to remote on 0f0c001 wanted to post the following: View the site here This comment will be updated on new commits. |
63270d1 to
2703079
Compare
|
Current PR attempt to solve two issues:
The first issue is addressed by setting a reasonable The second issue is resolved by using PETSc solver parameters ( For val-2f, the second issue is temporarily deferred, as the new PETSc solver parameters do not fully resolve it. Specifically, the val-2f_light_csv test case exhibits a convergence issue when running with Besides, recover failures are temporarily ignored for now. Please let me know if you have any comments! |
|
@lin-yang-ly I have been dealing with some similar brittleness for val-2i in #325. I might look at your PR for inspiration, though I think we're doing some similar things already in my most recent commits for that case. Might ask you some questions if I keep running into problems. |
Yes, just let me know if you have any questions! |
simopier
left a comment
There was a problem hiding this comment.
Some minor comments and changes, and this additional request:
In the val-2c documentation, the 67.07% RMSPE value in the text for the calibrate results should be updated to 67.09% (not consequential, but still, let's keep it consistent).
|
@lin-yang-ly in case you didn't see it; https://civet.inl.gov/job/3673706/: |
loganharbour
left a comment
There was a problem hiding this comment.
I'm still getting a diff with the petsc update (idaholab/moose#32404) + these changes:
test:val-2c.val-2c_delay_csv: Running csvdiff: /data/harblh/worktrees/moose/fix-cuda-typo/python/mooseutils/csvdiff.py /data/harblh/projects/tmap8/test/tests/val-2c/gold/val-2c_delay_csv.csv /data/harblh/projects/tmap8/test/tests/val-2c/val-2c_delay_csv.csv --relative-tolerance 3e-05 --abs-zero 1e-08
test:val-2c.val-2c_delay_csv: ERROR: In file val-2c_delay_csv.csv: The values in column "t2_paint_inventory" don't match.
test:val-2c.val-2c_delay_csv: relative diff: 9.033e-05 ~ 9.033e-05 = 6.019e-05 (6.019e-05)
Can we change that tolerance?
Yes, we can change |
|
Job Build test summary, step Build test summary on ca367b7 wanted to post the following: Test summaryCompared against c837769 in job civet.inl.gov/job/3665797. No change |
This comes from the new version of PETSc (not available yet). So you won't be able to reproduce it. Once the new moose update goes through (it won't update in TMAP8 cause TMAP8 fails with it), I will update the moose submodule here and we can run it with the new moose environment. |
simopier
left a comment
There was a problem hiding this comment.
This will be good to go if it passes the tests once @loganharbour adds the latest version of moose here.
Thank you!
(Ref. idaholab#380)
Co-authored-by: Pierre-Clement Simon <pierreclement.simon@gmail.com>
276589c to
b0bf4d0
Compare
|
@lin-yang-ly this has the new MOOSE submodule. Before working with this branch (because I had to force push to bring things up to date), you should fetch from your own remote and reset to the new branch. You'll also need to update your conda environment to work with this. Let me know if you need any help. |
|
we should just skip val-2f here as it will be fixed in #385 |
| abs_zero = 1e-8 | ||
| abs_zero = 2e-4 |
There was a problem hiding this comment.
I think this is too big of a bump. Need some other way to make the test perform consistently
| rel_err = 2e-5 | ||
| max_parallel = 1 # see #200 | ||
| recover = false # see #196 | ||
| abs_zero = 2e-4 |
There was a problem hiding this comment.
I would revert the large changes here and just skip for now while waiting on #385
| cli_args = 'Executioner/nl_rel_tol=6e-6 InterfaceKernels/interface_reaction_YHx_PCT/silence_warnings=true' # should be removed once PCT curve implementation for low pressure is complete | ||
| csvdiff = YHx_PCT_out.csv | ||
| max_parallel = 1 # see #200 | ||
| recover = false # see #196 |
There was a problem hiding this comment.
If we have to add this, then we're still not fixed
| Executioner/TimeStepper/growth_factor=1.2 | ||
| InterfaceKernels/interface_reaction_YHx_PCT/silence_warnings=true' # should be removed once PCT curve implementation for low pressure is complete | ||
| csvdiff = YHx_PCT_T1173_P5e4_out.csv | ||
| recover = false # see #196 |
| InterfaceKernels/interface_reaction_YHx_PCT/silence_warnings=true' # should be removed once PCT curve implementation for low pressure is complete | ||
| csvdiff = YHx_PCT_T1273_P3e3_out.csv | ||
| max_parallel = 1 # see #200 | ||
| recover = false |
|
Trying to understand why this was merged with over 2 week old testing? 😅 |
|
And with 2/11 tests passing |
|
So no one clicked the merge button on this PR? Well done tracking GitHub! |
As I know, no one clicked it, and TMAP8 only have the merge history of #385. |
|
That's right, I merged the other one and then came to this one to close it, only to find it "merged" already - by me. I was even more surprised because the discussions are also not resolved, the tests were not passing... but since everything was addressed and fixed in #385, it's all good. So Lin is right, Github made the link and closed this one too. I didn't even know it could do that. |
|
well, all's well. Good work all |
(Ref. #380)