Failing landice_FO_GIS_CoupledThickness test

ikalash commented 1 week ago

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 commented 1 week ago

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

bartgol commented 1 week ago

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 commented 6 days ago

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

jewatkins commented 6 days ago

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

bartgol commented 6 days ago

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 commented 6 days ago

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

sandialabs / Albany

Failing landice_FO_GIS_CoupledThickness test #1085