Introduce "rigorous" mode for convergence checking

[jwallwork23]

If rigorous=False then we get the existing behaviour where element, QoI, or estimator convergence implies termination of the fixed point iteration. If rigorous=True then all three conditions are required. Moreover, the check_convergence array is always True everywhere in rigorous mode.

This PR also changes the intermediate meaning of the MeshSeq.converged attribute slightly. Until convergence, it now relates specifically to whether element count has converged on each subinterval, whereas QoI and estimator convergence are summarised by single bools. If rigorous=False and either QoI or estimator convergence is detected, then we set convergence[:] = True.

Closes #53.

[jwallwork23]

@ddundo @acse-ej321 If you accept my invite to collaborate on Goalie then I'll add you as reviewers :)

[ddundo]

Thanks Joe! Before I review, just wanted to bring this issue up again. I haven't heard back from you but I still think it would make sense to make this be the default behaviour. The user can still take the current approach from inside the adaptor function - rather than vice versa. What do you think?

[jwallwork23]

Sorry @ddundo, after being on holiday I'd forgotten you wanted me to follow up on this. Will do.

[jwallwork23]

Okay, added some code that should address the issue. Closes #21.

[ddundo]

Thanks a lot Joe! I left one comment and have a more general comment:

This rigorous parameter sounds a bit unintuitive to me and maybe even misleading, as if otherwise it's not rigorous, whereas I assume the rigour might be quite problem-dependent. So how about calling this parameter something like convergence_criteria which can take, e.g., 'any' (default), 'all' (i.e. this "rigorous" approach), and maybe even only 'element', 'qoi', 'estimator' if the user only cares about a specific one to be satisfied.

Further to this, maybe we could add an optional get_qoi kwarg to MeshSeq also, which would only be used in MeshSeq.fixed_point_iteration to evaluate the qoi and check for this "rigorous" convergence criteria if the user wants it so.

[jwallwork23]

Yeah fair point, @ddundo. How about "safe" instead of "rigorous"? Or something like that? If not, we could just take the clearer any/all route.

[jwallwork23]

I don't think it would make sense to add a QoI related option to MeshSeq, since it has no concept of QoI. I guess the difference in the way I implemented "rigorous" mode so far for MeshSeq is that it (should) turn off the optimisations for early subintervals dropping out.

[ddundo]

The any/all naming sounds the most intuitive to me, but please choose what you think is best! Was just a side thought.

And I see what you mean now. I think about this "rigorous" approach and dropping out early subintervals as completely separate - I only brought it up here because it's affecting the same piece of code here. I kind of see the latter as almost necessary, as the solutions may be changed significantly at later intervals (which might have an earlier converged status).

And yeah, I agree that including anything qoi-named is a bit clumsy in MeshSeq. In my examples I've noticed that, coincidentally, the element convergence was reached, but the quantities I was interested in (ice volume or position of the grounding line) would change significantly. So I was keeping track of this separately inside the adaptor function.

[jwallwork23]

The any/all naming sounds the most intuitive to me, but please choose what you think is best! Was just a side thought.

Okay, will go with any/all.

And I see what you mean now. I think about this "rigorous" approach and dropping out early subintervals as completely separate - I only brought it up here because it's affecting the same piece of code here. I kind of see the latter as almost necessary, as the solutions may be changed significantly at later intervals (which might have an earlier converged status).

Sorry, what I meant to say was that in "rigorous" mode, there is no drop-out at all. i.e., we terminate only if all the convergence criteria are met on all the subintervals. Whereas for "non-rigorous" mode, we allow dropping out of early subintervals (but not later ones) and only require one convergence criterion for termination.

And yeah, I agree that including anything qoi-named is a bit clumsy in MeshSeq. In my examples I've noticed that, coincidentally, the element convergence was reached, but the quantities I was interested in (ice volume or position of the grounding line) would change significantly. So I was keeping track of this separately inside the adaptor function.

Perhaps we could add a convergence checker for custom quantities other than the QoI? (The problem with allowing multiple QoIs would be that we would then need to solve multiple versions of the adjoint problem.)

[jwallwork23]

Okay I think that's ready for review now @ddundo. I decided to separate out the drop_out_converged as a separate parameter to give more control.

[ddundo]

Thanks again Joe! I am still trying to process some things... I will think until tomorrow and maybe ask you a thing or two during the meeting if it's still unclear.

But one thing which I'd mention immediately... Could you help me understand how this dropping out of early converged intervals would work with space-time normalisation? That is, let's say that we have 2 subintervals and the first one has converged. But in the next fixed point iteration, the solution at the 2nd interval would still affect the 1st interval through space-time normalisation, right? So we should still adapt the 1st interval in that case? Or is this irrelevant?

[jwallwork23]

But one thing which I'd mention immediately... Could you help me understand how this dropping out of early converged intervals would work with space-time normalisation? That is, let's say that we have 2 subintervals and the first one has converged. But in the next fixed point iteration, the solution at the 2nd interval would still affect the 1st interval through space-time normalisation, right? So we should still adapt the 1st interval in that case? Or is this irrelevant?

Space-time normalisation would affect the metric for the first subinterval, yes. "Dropping out" means that we are ignoring that metric information and keeping the adapted mesh we already have. So yes, it's possible that dropping out will mean we don't end up meeting the target space-time metric complexity because of issues like these. I guess it's a trade-off. We can put the default drop_out_converged=True if you'd prefer to make this optional?

[jwallwork23]

@ddundo happy for this to be merged?

[ddundo]

Sorry, yes - looks good to me! I thought we agreed on the meeting so I didn't comment further :)