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

Failing landice_FO_GIS_CoupledThickness test #1085

Open
ikalash opened this issue Oct 22, 2024 · 8 comments
Open

Failing landice_FO_GIS_CoupledThickness test #1085

ikalash opened this issue Oct 22, 2024 · 8 comments

Comments

@ikalash
Copy link
Collaborator

ikalash commented Oct 22, 2024

This test began a couple weeks ago with diffs:

https://sems-cdash-son.sandia.gov/cdash//test/8919018?graph=status

The diffs are tiny actually:

Response Test 0: 1.094731601407e+08 != 1.096541803121e+08 (rel 1.000000000000e-06 abs 1.000000000000e-06)

Sensitivities (0,0):

                    1.829216632090e+07
Sensitivity (0,0), Test 0,0: 1.829216632090e+07 != 1.829143517389e+07 (rel 1.000000000000e-06 abs 1.000000000000e-06)

Presumably these are due to a PR Luca pushed a couple weeks ago.

The diffs are very small. Should we just rebaseline?

@jewatkins
Copy link
Collaborator

reblessing seems okay to me if @bartgol doesn't see anything obvious in his PR.

@bartgol
Copy link
Collaborator

bartgol commented Oct 23, 2024

The PR that removed warnings has nothing that looks suspicious. And yet, the tests started to fail right after that PR was merged. I'm trying to bisect on that PR, to see if I'm missing something. The diffs are indeed minor, but the fact that nothing should have changed bothers me.

@jewatkins
Copy link
Collaborator

Maybe it was a Trilinos change?
old: 7b63078f259
new: 83e06e659ac

@jewatkins
Copy link
Collaborator

the only thing I see that might be relevant is the stk snapshot

@bartgol
Copy link
Collaborator

bartgol commented Oct 23, 2024

I just ran git bisect, so I kept trilinos fixed to whatever I have on my laptop, and it pin pointed this commit:

b7ebca3a86bbdfe48987be48ed81b6afe058a38c is the first bad commit
commit b7ebca3a86bbdfe48987be48ed81b6afe058a38c
Author: Luca Bertagna <lbertag@sandia.gov>
Date:   Mon Oct 7 11:51:12 2024 -0600

    Fix misc warnings borderline buggy

 src/disc/omegah/Albany_OmegahMeshFieldAccessor.cpp | 2 ++
 src/evaluators/gather/PHAL_GatherSolution_Def.hpp  | 6 ++++--
 src/landIce/evaluators/LandIce_SimpleOperation.hpp | 2 +-
 tests/unit/disc/generic/DummyConnManager.cpp       | 6 +++---
 4 files changed, 10 insertions(+), 6 deletions(-)

I thought none of these changes mattered for FOThickness, but it's not true:

  • the first and last don't, since they are not used
  • the gather solution fix only matters marginally: we were allocating blah_dot and blah_dotdot also for non time dep problems, so not a big deal either way
  • the SimpleOperation evaluator DOES matter. There was a bug in the BinarySum op, where instead of doing result = x+y we just did result=x.

So we were actually using H0 (the initial thickness) for other parts of the calculations (in the velocity solver). This is an example of why warnings are not to be taken lightly...

I think we can bless and close this issue.

@mperego
Copy link
Collaborator

mperego commented Oct 23, 2024

Awesome. Thanks all for the sleuthing. And for fixing the BinarySum op.

@jewatkins
Copy link
Collaborator

Did anyone rebless? this is still failing.

@bartgol
Copy link
Collaborator

bartgol commented Nov 4, 2024

Ah, no. I can set the correct values today and push the fix to master.

Edit: done in d178ff6

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

No branches or pull requests

4 participants