Split timestepper #537
Merged
Split timestepper #537
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tommbendall
added
the
enhancement
tommbendall
requested changes
Sep 2, 2024
| @@ -130,4 +130,4 @@ def test_boussinesq(tmpdir, compressible): | |||
|
|
|||
| # Slack values chosen to be robust to different platforms | |||
| assert error < 1e-10, f'Values for {variable} in ' + \ | |||
| 'Incompressible test do not match KGO values' | |||
| '{compressible} test do not match KGO values' | |||
There was a problem hiding this comment.
Suggested change
| '{compressible} test do not match KGO values' | |
| f'{compressible} test do not match KGO values' |
| # If we have physics schemes, extract these. | ||
| if 'physics' in term_splitting: | ||
| if physics_schemes is None: | ||
| raise ValueError('Physics schemes need to be specified when applying a physics splitting in the SplitTimestepper') |
There was a problem hiding this comment.
Could this be split over multiple lines?
| if term == 'physics': | ||
| count += weights[idx] | ||
| if count != 1: | ||
| raise ValueError('Incorrect weights are specified for the physics schemes in the split timestepper.') |
|
|
||
| # Multilevel schemes are currently not supported for the dynamics terms. | ||
| for label, scheme in self.dynamics_schemes.items(): | ||
| assert scheme.nlevels == 1, "Multilevel schemes are not currently implemented in the split timestepper" |
There was a problem hiding this comment.
And here, maybe by doing:
Suggested change
| assert scheme.nlevels == 1, "Multilevel schemes are not currently implemented in the split timestepper" | |
| assert scheme.nlevels == 1, \ | |
| "Multilevel schemes are not currently implemented in the split timestepper" |
| # dynamics scheme | ||
| for term in self.term_splitting: | ||
| if term != 'physics': | ||
| assert term in self.dynamics_schemes, f"The {term} terms do not have a specified scheme in the split timestepper" |
There was a problem hiding this comment.
and here ... e.g.
Suggested change
| assert term in self.dynamics_schemes, f"The {term} terms do not have a specified scheme in the split timestepper" | |
| assert term in self.dynamics_schemes, \ | |
| f"The {term} terms do not have a specified scheme in the split timestepper" |
| dynamics_schemes = {'transport': ImplicitMidpoint(domain), | ||
| 'diffusion': ForwardEuler(domain)} | ||
| term_splitting = ['transport', 'diffusion', 'physics'] | ||
| stepper = SplitTimestepper(equation, term_splitting, dynamics_schemes, io, spatial_methods=spatial_methods, physics_schemes=physics_schemes) |
There was a problem hiding this comment.
can we also split this line?
| 'diffusion': ForwardEuler(domain)} | ||
| term_splitting = ['diffusion', 'transport', 'physics', 'transport'] | ||
| weights = [1., 0.6, 1., 0.4] | ||
| stepper = SplitTimestepper(equation, term_splitting, dynamics_schemes, io, weights=weights, spatial_methods=spatial_methods, physics_schemes=physics_schemes) |
| dynamics_schemes = {'transport': SSPRK3(domain), | ||
| 'diffusion': SSPRK3(domain)} | ||
| term_splitting = ['physics', 'transport', 'diffusion', 'physics'] | ||
| weights = [1./3., 1, 1, 2./3.] |
| 'diffusion': SSPRK3(domain)} | ||
| term_splitting = ['physics', 'transport', 'diffusion', 'physics'] | ||
| weights = [1./3., 1, 1, 2./3.] | ||
| stepper = SplitTimestepper(equation, term_splitting, dynamics_schemes, io, weights=weights, spatial_methods=spatial_methods, physics_schemes=physics_schemes) |
| term_splitting (list): a list of labels specifying the terms that should | ||
| be solved separately and the order to do so. | ||
| dynamics_schemes: (:class:`TimeDiscretisation`) A list of time | ||
| discretisations for the dynamics schemes. A scheme must be |
There was a problem hiding this comment.
these continuation comments need indenting -- the pattern should be:
Args:
variable_name (variable_type): description ....
... continued description ...
... more continued description ...
next_variable (next_type): etc ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a general SplitTimestepper class that allows different terms to be timestepped separately using a defined time discretisation. A 'term_splitting' list specifies the order that the terms are solved, e.g ['transport', 'physics', 'diffusion']. A timestepping scheme for each non-physics term is specified in the 'dynamics_schemes' dictionary.
Furthermore, substepping can be used for a term by defining weights. For example, we can Strang-split the transport between diffusion with term_splitting=['transport','diffusion','transport'], weights=[0.5,1,0.5].
A test is added to integration_tests/model/test_split_timestepper.py. This tests three different splittings of the advection-diffusion equation with source physics.