Appl lens2pexercise3 in dumux-lecture fails after commit updating the timeloop

Possible bug in the timeloop for large time step sizes? Fails since 2fbb615b

changed milestone to %3.1

changed the description

To me this looks like we need one additional timestep now and therefor compare against the wrong .vtu file.

Anyways, this might still be a bug or due to some changes from the timeloop.

I think the first check point is not exactly reached!? the changes in the timeloop are not supposed to change anything in the functionality. If they do, it would be a bug.

so just to summarize what we found out: The advance time-step was moved and our check for whether it is a CheckPoint or not was/is not really accurate for large timeSteps and Checkpoints as we can see from this test in general.

The checkpoints are not actually met (e.g. 39400778 instead of 40000000). This was and still is an issue. I tried changing the epsilon in the compare from 1e-8*dt to 1e-5*dt/nextTime where nextTime is time+dt just because that was the only thing that was always available and should never be 0. If I thought correctly this should give as an epsilon of at least 1e-5 or potentially lower. It could easily be 1e-10 or 15 and the question is: Do we run into other problems there?

@timok: I am running tests right now just for fun, but have to leave now. For lecture the changes actually force the correct checkpoints but make the lens2pexercise3 test fail because we use different timesteps now.

I forgot to post the update.

While the proposed changes work in a way, I am still wondering whether the new version is actually beneficial or if we should do something else for the epsilon. I could just push the update and adapt the references for the lecture part because in my opinion this actually fixes the "bug" of not reaching the wanted CheckPoints exactly.

so I get the following tests that fail, where all of them for dumux-master also fail without the changes at the moment.

The following tests FAILED:
         72 - test_gridmanager_gmsh_3d_alu_parallel (Timeout)
        202 - test_1p_periodic_tpfa_parallel (Failed)
        212 - test_1p_gstat (Not Run)
        253 - test_2p_cornerpoint (Not Run)
        273 - test_impeswithamg (Failed)
        337 - test_richards_lens_box_parallel_yasp (Failed)
        338 - test_richards_lens_tpfa_parallel_yasp (Failed)
        339 - test_richards_lens_tpfa_parallel_yasp_restart (Failed)
        340 - test_richards_lens_box_parallel_ug (Failed)
        341 - test_richards_lens_tpfa_parallel_ug (Failed)
        342 - test_richards_lens_box_parallel_alu (Failed)
        343 - test_richards_lens_tpfa_parallel_alu (Failed)

For lecture things are different:

The following tests FAILED:
          3 - lens2pexercise3 (Failed)
         17 - henry2p (Failed)

lens2pexercise3 now has a slightly different solution since the CheckPoints are matched and the timesteps are different. Henry2p should only write 2 outputs (0 and 1e9) but used to write more until now (which was wrong).

The course passes expect for the coupling things that do not compile.

@scholz I think the usage of Dune::FloatCmp::eq is simply wrong. The epsilon is already used as eps in the expression

abs(first-second) <= epsilon * max(abs(first), abs(second))

which means it is already a relative epsilon. Replacing 1e-8*dt by a plain 1e-7 should be fine. Can you try that?

edit: Your solution which dt/nextTime seems arbitrary to me. Also dt can be a lot smaller than nextTime. But I think in the lecture test as dt~nextTime this actually results in an epsilon of almost constant 1e-5 in your case.

I'll open an MR

mentioned in merge request !1552 (merged)

The solution was absolutely arbitrary, that is why I was not happy with it ;-)

But the solution now seems reasonable and I'll look at the MR. Thanks!

closed via merge request !1552 (merged)

mentioned in commit afa36fdf