Fix Solver Tests, addressing #156

[ncdorn]

Current situation

Currently the tests for svZeroDSolver do not actually test the relative difference between the reference solution and the test result, as outlined in #156 . This was due to a syntax error, specifically improper use of np.all() in the solver tolerance check. This PR updates the testing protocol to ensure that every value of the reference solution is being compared to the test result, ensuring that the relative difference between the two values is below the desired tolerance

Release Notes

• testing updated using merged DataFrames and booleans
• error messages for failing tests are improved to show the field, result and reference values and in some cases the relative difference between the two values.

Documentation

no Documentation to add

Testing

Test tolerances were increased from 1e-8 to 1e-7. For coupledBlock_closedLoopHeart_withCoronaries.json the test tolerance was increased to 1e-1 due to increased numerical stiffness and differences in the result between operating systems. After these changes, all tests are passing.

Code of Conduct & Contributing Guidelines

• I agree to follow the Code of Conduct and Contributing Guidelines.

[ncdorn]

so if we update the test tolerance for flow results from 1e-8 to 1e-7, all tests pass. Currently the pressure tolerance is already set at 1e-7. Is this something we want to do @mrp089? you could check previous workflow runs to see that the flow result and reference are very close for closedLoopHeart_singleVessel.json, the only test that fails against a 1e-8 flow tolerance

[mrp089]

Can you tighten the solver tolerances? We're probably testing with the default value of 1e-8. Do the tests pass on all machines if you go, e.g., to 1e-12 and recompute the reference solutions?

[ncdorn]

Can you tighten the solver tolerances? We're probably testing with the default value of 1e-8. Do the tests pass on all machines if you go, e.g., to 1e-12 and recompute the reference solutions?

So I can't tighten the solver tolerance below 1e-9 without certain test cases hitting the maximum number of nonlinear iterations. When I use solver tolerance 1e-9, with test tolerance 1e-8, the tests still do not pass on ubuntu or macOS 13. see latest commit + workflow runs for details

[mrp089]

That's good to know, @ncdorn. That means we're testing as accurately as we can. @menon-karthik, do you have any idea why those tests are particularly trickier?

• coupledBlock_closedLoopHeart_withCoronaries
• coupledBlock_closedLoopHeart_singleVessel
• closedLoopHeart_singleVessel

If this does not indicate some error, we'll loosen the tolerances.

[ncdorn]

@menon-karthik just bumping this as it is currently a bottleneck to the other pull requests

[mrp089]

@ncdorn, can you also add a test readme, similar to https://github.com/SimVascular/svMultiPhysics/tree/main/tests

[ncdorn]

@ncdorn, can you also add a test readme, similar to https://github.com/SimVascular/svMultiPhysics/tree/main/tests

see latest commit!

[mrp089]

@menon-karthik, can you have a look at why those test cases are having problems on different machines:

• coupledBlock_closedLoopHeart_withCoronaries
• coupledBlock_closedLoopHeart_singleVessel
• closedLoopHeart_singleVessel
  It's okay if we need to lower the tolerance from 1e-8 to 1e-7, but it would be good to understand why it's specifically those tests (could be something related to how that relative norm is calculated?). Thanks!

[mrp089]

@ncdorn, in the interest of getting this (and the other PRs) merged, can you go ahead and lower the tolerance to 1e-7?

[ncdorn]

so the latest commit is passing on all operating systems except for windows. Looks like again, a very small difference between reference and computed solution for the closedLoopHeart_singleVessel.json case

[mrp089]

Thanks, @ncdorn! Looking at the results, it seems we would need to coarsen the test tolerance to 1e-5 to make that test pass on Windows, which would be a big step from 1e-8.

I could crank up to "absolute_tolerance": 1e-12 in closedLoopHeart_singleVessel.json (going in increments of 0.1). Can you rerun the test with that?

To make our tests more robust, can you go through every test case and set absolute_tolerance to the tightest tolerance for which the Newton solver still converges at all time steps? Usually, there is a sharp drop-off going from around 2-4 average iterations per time step to not converging (in "maximum_nonlinear_iterations": 30). Currently, we're not using the option absolute_tolerance in any of the test cases. For fun, you could also set maximum_nonlinear_iterations in one of the tests (even if it's just to the default value).

This ensures that every result we test is computed as precisely as possible. It's a bit tedious, but this should save us from most future machine-dependent troubles (we also did that for svMultiPhysics). Fingers crossed we're done with this PR after that!

[ncdorn]

unexpectedly, this leads to failure on the other operating systems. super confusing. Will keep looking into it.

[ncdorn]

ok so what I've done is adjust the input time arrays in the external_solver_coupling_blocks such that there should be no interpolation needed, which could lead to discrepancies in operating systems. This got coupledBlock_closedLoopHeart_singleVessel.json to pass on some operating systems but the withCoronaries case still does not pass. I am going to try reducing the number of cardiac cycles from 20 to 10 in case rounding/computation differences between operating systems are compounding over many cardiac cycles.

[ncdorn]

so I converted the input flow in the external solver coupling blocks to steady flow, and still the tests do not pass, implying that it is not an issue with the interpolation, but maybe somewhere in the numerics, or in the compilation of the 3d-0d interface.

[menon-karthik]

Hi @ncdorn sorry I was AWOL on this! I just looked through and caught myself up with what's happening here.

The closed loop block is numerically stiff because it has a lot of different components acting together. In particular, I think the valves make it sensitive (uninitialized variables can be slightly different on different operating systems).

I believe the coupledBlock_closedLoopHeart_singleVessel.json both test the same functionality, i.e. the blocks that are used to couple with other solvers (external_solver_coupling_blocks). Can you please verify that the code coverage is indeed exactly the same with and without the _withCoronaries test? If it is, then we are not testing anything extra with the latter, just creating an even more stiff system by adding extra coronary blocks to the test.

So if the code coverage is the same with and without the _withCoronaries test I suggest we either remove the _withCoronaries test or increase the tolerance for only that test if you don't want to remove it. I might prefer the second option so that we are still testing a few stiff systems but we are not being unrealistic in the tests.

[ncdorn]

Alright so I had to reduce the test tolerance to 1e-1 only for coupledBlock_closedLoopHeart_withCoronaries.json. Now all tests are passing. let me know what you all think and if there are further changes that need to be made. I am not sure how to generate a coverage report for a single pull request commit, may have to merge it in to see the coverage numbers, unless one of you know how to do that.

[ncdorn]

@mrp089 what do you think of this? Any other changes we need to make?

[ncdorn]

just bumping this in case anyone is able to take a look so we can merge this in soon and move on to the other issues @mrp089 @menon-karthik

[mrp089]

Sorry, @ncdorn. I thought I had submitted those comments

[ncdorn]

no worries, thanks for checking! changes addressed.