Get SUPG working for MixedFunctionSpace #580
Conversation
… to see if SUPG still works
| @@ -163,9 +163,9 @@ def __init__(self, base_scheme, domain, M, maxk, quad_type, node_type, qdelta_im | |||
|
|
|||
| # Get Q_delta matrices | |||
| self.Qdelta_imp = genQDeltaCoeffs(qdelta_imp, form=formulation, | |||
| nodes=self.nodes, Q=self.Q) | |||
| nodes=self.nodes, Q=self.Q, nNodes=M, nodeType=node_type, quadType=quad_type) | |||
There was a problem hiding this comment.
Just to check, is this a change that should be part of this PR?
There was a problem hiding this comment.
Nothing gets past you.. they are required for the parallel preconditioners, so thought I would just get it onto main. Can remove and put in a separate PR?
| self.wrapper.setup(field_name) | ||
| new_test = self.wrapper.test | ||
| self.residual = self.residual.label_map( | ||
| lambda t: t.get(prognostic) == field_name and t.has_label(transport), |
There was a problem hiding this comment.
I generally don't like specific terms appearing in this file because I think the time_discretisation should be agnostic to the type of term being solved...
... if we need to specifically only replace the test function in the transport terms, could the SUPGOptions object contain a list of labels for the terms that need replacing? Then the user can specify which terms are going to be adjusted
There was a problem hiding this comment.
Okay agreed, I have added a list of term labels to cycle through. I need to add a test for this so hang fire for that
|
|
||
| # -------------------------------------------------------------------- # | ||
| # Make boundary conditions | ||
| # -------------------------------------------------------------------- # | ||
|
|
||
| if not apply_bcs: | ||
| self.bcs = None | ||
| elif self.wrapper is not None: | ||
| elif self.wrapper is not None and self.wrapper_name != "supg": |
There was a problem hiding this comment.
Is this amendment to the if statement definitely still needed? (I'm happy to keep it if it is)
| @@ -381,9 +387,13 @@ def setup(self): | |||
| # -------------------------------------------------------------------- # | |||
| # Set up test function | |||
| # -------------------------------------------------------------------- # | |||
| if self.time_discretisation.wrapper_field_names is not None: | |||
| uadv = Function(domain.spaces('HDiv')) | |||
There was a problem hiding this comment.
could this line be above the if statement?
| @@ -101,6 +101,10 @@ def __init__(self, domain, field_name=None, solver_parameters=None, | |||
| elif self.wrapper_name == "recovered": | |||
| self.wrapper = RecoveryWrapper(self, options) | |||
| elif self.wrapper_name == "supg": | |||
| if options.field_names: | |||
There was a problem hiding this comment.
Could this be if options.field_names is not None:
There was a problem hiding this comment.
I've instead just done it as self.wrapper_field_names = options.field_names and the None check is now further down
gusto/core/configuration.py
Outdated
| ibp = IntegrateByParts.TWICE | ||
| field_names = [] |
There was a problem hiding this comment.
It might be more natural to have None as the default option here
Co-authored-by: Thomas Bendall <[email protected]>
gusto/core/configuration.py
Outdated
| @@ -202,6 +202,7 @@ class SUPGOptions(WrapperOptions): | |||
| tau = None | |||
| default = 1/sqrt(15) | |||
| ibp = IntegrateByParts.TWICE | |||
| suboptions = None # Dictionary containing field_name and term_labels | |||
There was a problem hiding this comment.
As well as fixing the lint, can you expand on this comment just to make it a bit clearer what suboptions should be, i.e. the keys of the dictionary should be strings for the names of the fields, and the values should be a list of terms to apply the SUPG test function to
Currently SUPG does not work for mixed function spaces. Here I have allowed for the altering of multiple test functions.
Note that MixedFSWrapper is not used, as altering a single test function in the tuple of test functions for a MixedFunctionSpace was difficult to do.
There is scope in future to not use SUPG as a wrapper, but this is not addressed in this pull request.
