Cory Adams Moulton (V2) #10

cmikida2 · 2020-10-27T19:58:13Z

This pull request takes the single-rate component of the implicit Adams development associated with this pull request and splits it into a separate PR, while also attempting to fix a number of documentation and refactoring issues raised in the initial PR. Among the largest changes are

Adding a base class for Adams methods to remove duplicate code.
Adding a number of utilities to that base class to remove duplicate code, largely associated with history rotation.
Moving the implicit solve used by both RK and Adams methods to implicit.py.
Refactoring the generate_butcher function in the RK base class to allow for Adams methods to use this function for bootstrapping.
Modifying the simple convergence testing utility to allow for implicit testing, and folding Adams-Moulton testing into test_ab.py (test_ab.py -> test_adams.py).
Adding a lot of documentation, but I am still iterating on this and trying to get this part exactly right.

- also adds diagonally implicit RK methods

base class

encompass both AB and AM method tests

inducer · 2020-11-16T04:29:48Z

leap/implicit.py

+
+def generate_solve(cb, unknowns, equations, var_to_unknown, guess):
+
+    # got a square system, let's solve


Suggested change

# got a square system, let's solve

assert len(unknowns) == len(equations)

?

This change has been added in ae9d04f.

inducer · 2020-11-16T04:32:41Z

leap/rk/__init__.py

+    recycle_last_stage_coeff_set_names = ()
+
+
+class DIRK2MethodBuilder(ImplicitButcherTableauMethodBuilder):


Are these methods being tested somewhere?

This is addressed in 6d93fee, which adds accuracy tests for these methods to the RK testing suite.

inducer · 2020-11-16T17:07:57Z

leap/multistep/__init__.py

@@ -219,7 +222,7 @@ class AdamsBashforthMethodBuilder(MethodBuilder):
    """

    def __init__(self, component_id, function_family=None, state_filter_name=None,
-            hist_length=None, static_dt=False, order=None):
+            hist_length=None, static_dt=False, order=None, _extra_bootstrap=False):


I'm not liking this _extra_bootstrap thing, mostly because it's ill-defined.

What happens with the extra history after bootstrap?

What are its semantics in the explicit case?

The justification for it seems really weak and mostly focused on debugging?

This is an option that was previously in place to pass a certain test that compared multi-rate IMEX to single-rate implicit. Given that that's not what we're talking about here, this is an artifact of outdated development and needs to be removed. I'm on it.

This option and its artifacts are removed by ae9d04f.

leap/multistep/__init__.py

inducer · 2020-11-16T17:18:14Z

leap/multistep/__init__.py

+
+        # {{{ emit solve if possible
+
+        if unknowns and len(unknowns) == len(equations):


What is the else to this if? Why is it OK to just go by here if we can't solve?

The else - as far as I'm aware - would be the fully explicit case, but at bare minimum in that case the if condition isn't indicative of that.

Hmm. I realize now, though, that I only have this in the Adams-Moulton class, so explicit isn't a concern. Let me think about contingencies/what should be done if we don't have a square system, or if it's even possible for that to happen with the current setup.

Addressed by 3803398.

Non-square systems aren't allowed IMO.

When you say "not allowed" do you mean they should never happen? I added the elifs (and now an else) to this block to catch instances in which a system is non-square, but I'm not sure if you're saying that that's not sufficient, or if the check for a square system isn't needed at all because we should never not have one.

inducer · 2020-11-16T17:22:01Z

leap/multistep/__init__.py

+            if self.extra_bootstrap:
+                save_crit = i+1
+            else:
+                save_crit = i
+
+            with cb.if_(self.step, "==", save_crit):
+                cb(self.history[i], rhs_next_var)
+
+                if not self.static_dt:
+                    cb(self.time_history[i], self.t + self.dt)


This code seems like it's an adverse consequence of the awkward redefinition of "history".

That and the extra bootstrapping. Given that it's a function of those two things, I'll need to think about how those things need to change before addressing this.

This is addressed along with redefinition of history by 4a87ba9.

generate_solve

bootstrap logic associated with this, and slightly modifies testing threshold to acquiesce barely-failing extended-history AM

that should never be encountered

cmikida2 · 2020-11-23T20:06:39Z

I've addressed all of your comments above and made a number of changes to the Adams-Moulton code, and while most of them should explain themselves as direct responses to the comments above, I want to also mention that I had to very slightly twiddle the convergence tolerance for the accuracy tests in order for the implicit extended-history tests to pass. The pass criteria was changed from 0.9 * expected_order to 0.89 * expected_order, as the EOCs for these cases were one or two hundredths places away from the initial target. The one-line code change for this is in 4a87ba9.

I have checked that the addition of extended history has the same effect on EOC relative to the square system as in the case of Adams-Bashforth - in both cases, there's a slight drop in EOC, typically on the order of 0.08-0.1. I'm not overly concerned about this, but happy to discuss further, as I don't think you were all too thrilled with the fudge factor that was being applied to begin with.

inducer

Thanks! Once the wrinkles below are addressed, I think this is in good shape.

leap/implicit.py

inducer · 2020-12-04T16:53:28Z

leap/multistep/__init__.py

-            with cb_bootstrap.if_(self.step, "==", self.hist_length):
+            bootstrap_length = self.hist_length
+            with cb_bootstrap.if_(self.step, "==", bootstrap_length):


Yeah, this was removed in 61277d8. An artifact of the old extend-bootstrap thingy that I overlooked.

inducer · 2020-12-04T16:54:46Z

leap/multistep/__init__.py

+            for i in range(self.hist_length):
+                cb(time_data_var[i], time_data[i] - self.t)
+
+            relv_times = time_data_var


Suggested change

relv_times = time_data_var

relevant_times = time_data_var

Addressed in 61277d8.

inducer · 2020-12-04T16:55:30Z

leap/multistep/__init__.py

+                               component_id=self.component_id,
+                               time_id="", time=self.t)
+
+    def set_up_time_data(self, cb, new_t):


Give me a docstring describing under which circumstances this is used, like pre/postcondition.

Added in 61277d8, but not sure if it was exactly what you're looking for. I went through the purpose of the function, described the argument, and what it takes in and returns. Happy to amend this docstring if it seems unsuitable.

inducer · 2020-12-04T16:58:35Z

leap/multistep/__init__.py

+            dt_factor = self.dt
+            t_end = 1
+
+        return time_data, relv_times, dt_factor, t_end


Explain difference between time_data and relevant_times.

I tried to add this to the docstring (added in 61277d8), but there are two main differences:

relevant_times subtracts out self.t, whereas time_data does not. I am truthfully not sure how critical this is.

more importantly, time_data is a list, whereas relevant_times is an array.

inducer · 2020-12-04T19:14:02Z

leap/multistep/__init__.py

+                    "than equations")
+        elif len(unknowns) < len(equations):
+            raise ValueError("Adams-Moulton implicit timestep has more equations "
+                    "than unknowns")


Is there an else to this last condition? If not:

Suggested change

"than unknowns")

"than unknowns")

else:

assert False

Addressed in 61277d8.

inducer · 2020-12-04T19:16:41Z

leap/multistep/__init__.py

+
+        # {{{ emit solve if possible
+
+        if unknowns and len(unknowns) == len(equations):


Non-square systems aren't allowed IMO.

inducer · 2020-12-04T19:25:03Z

leap/rk/__init__.py

-                        cb(my_rhs, last_rhss[name])
-                        make_known(my_rhs)
+        def make_known(v):
+            unknowns.discard(v)


Why is this here? Is there ever a scenario where a variable would be added to unknowns only to have to be removed later?

This is something that I pulled in from the existing Butcher generator. Truthfully I'm not sure that such a scenario exists, though based on where this function is called, it looks like it might be possible in the case of stage recycling. Let me run some tests to determine whether this is even needed, because it's a fair question.

Addressed in 61277d8. I've removed this function and replaced calls to it with adding rhs's to knowns, but I agree with you where we should never need to remove things from unknowns, and getting rid of that has no ill effect that I can see.

inducer · 2020-12-04T19:27:33Z

leap/rk/__init__.py

+                            "unknowns than equations")
+                elif len(unknowns) < len(equations):
+                    raise ValueError("Runge-Kutta implicit timestep has more "
+                            "equations than unknowns")


else assert False?

Addressed in 61277d8.

inducer · 2020-12-04T19:28:42Z

leap/rk/__init__.py

+    def generate_butcher_finish(self, cb, stage_coeff_set_names,
+            stage_coeff_sets, rhs_funcs, estimate_coeff_set_names,
+            estimate_coeff_sets, stage_rhs_vars, estimate_vars):


Explain the circumstances in which "finish" is invoked.

The reason that "finish" exists separately from "primary" essentially relates to the use of these functions in bootstrapping the multistep methods. This "finish" is invoked immediately after "primary" by regularly generated RK methods, because one of its primary responsibilities is yielding state and incrementing the time, but when bootstrapping a multistep method, this "finish" is not called, because other pieces of code are responsible for doing those things (not to mention there are instances in which other things need to occur in the "finish" of a bootstrap step for a multistep method pertaining to history updates).

Co-authored-by: Andreas Klöckner <[email protected]>

relv_times->relevant_times, else assert falses on solves

…into cory-adams-moulton

cmikida2 · 2021-08-31T20:10:49Z

@inducer I've just looked this over again and brought it up to speed with the main leap branch. It remains, as far as I can see, ready for a further look, or at the very least is in a good position for someone else to pick it up and make any further tweaks necessary.

I'll also note that other PRs for adding adaptive implicit-explicit Adams schemes (#11 and #13) depend upon this work, at least in part.

cmikida2 added 10 commits October 27, 2020 13:49

Adds implicit capability to convergence checking utility

a580cc3

Refactors generate_butcher for use for bootstrapping with Adams methods

4d53ad8

- also adds diagonally implicit RK methods

Moves implicit solve into implicit.py as a utility function

9e58682

Adds Adams-Moulton - both AM and AB now draw from common AdamsMethod

430ae0b

base class

Adds tests for Adams-Moulton, renames test_ab to test_adams to

2d46b59

encompass both AB and AM method tests

Remove excess poorly formatted documentation

2930f0c

More documentation for RK

bfce40a

Flake8 issues

41afd41

Add scipy to ci.yml so implicit tests run

a9511ec

Pylint fix for implicit RK

ba44c05

cmikida2 mentioned this pull request Oct 27, 2020

Cory implicit adams #4

Open

cmikida2 marked this pull request as draft November 2, 2020 18:05

cmikida2 marked this pull request as ready for review November 2, 2020 18:06

cmikida2 mentioned this pull request Nov 2, 2020

Embedded Adams w/Adaptive Timestepping #11

Draft

inducer reviewed Nov 16, 2020

View reviewed changes

cmikida2 added 6 commits November 16, 2020 11:53

Removes outdated extra_bootstrap option, makes suggested change to

ae9d04f

generate_solve

Add accuracy testing for DIRK methods

6d93fee

Fix bug in implicit order bootstrap mapping

1bc50be

Fixes "history" naming conventions in Adams methods, removes confusing

4a87ba9

bootstrap logic associated with this, and slightly modifies testing threshold to acquiesce barely-failing extended-history AM

Placate Flake8

0e03294

Address "else" conditions in solve, mostly by raising value errors

3803398

that should never be encountered

inducer reviewed Dec 4, 2020

View reviewed changes

cmikida2 and others added 2 commits December 4, 2020 13:51

Remove todo from generate_solve

4066a5e

Co-authored-by: Andreas Klöckner <[email protected]>

Review fixes: adding docstring to set_up_time_data,

61277d8

relv_times->relevant_times, else assert falses on solves

Base automatically changed from master to main March 8, 2021 05:09

cmikida2 added 4 commits August 31, 2021 14:16

Merge branch 'main' into cory-adams-moulton

a48dd1c

Merge branch 'cory-adams-moulton' of https://github.com/cmikida2/leap …

31fc9ad

…into cory-adams-moulton

Merge branch 'main' into cory-adams-moulton

10d9b94

Merge branch 'cory-adams-moulton' of https://github.com/cmikida2/leap …

afa13b2

…into cory-adams-moulton

Placate Flake8

4401fe8

cmikida2 mentioned this pull request Aug 31, 2021

Embedded adams combined #13

Draft


		def generate_solve(cb, unknowns, equations, var_to_unknown, guess):

		# got a square system, let's solve

	# got a square system, let's solve
	assert len(unknowns) == len(equations)

		recycle_last_stage_coeff_set_names = ()


		class DIRK2MethodBuilder(ImplicitButcherTableauMethodBuilder):


		# {{{ emit solve if possible

		if unknowns and len(unknowns) == len(equations):

Cory Adams Moulton (V2) #10

Are you sure you want to change the base?

Cory Adams Moulton (V2) #10

Conversation

cmikida2 commented Oct 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmikida2 commented Nov 23, 2020

inducer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmikida2 commented Aug 31, 2021