-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mixed order recovery #524
Mixed order recovery #524
Conversation
… mixed-order-recovery
… mixed-order-recovery
""" | ||
|
||
def __init__(self, domain): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs some interface documentation
Great - thanks @Witt-D ! We need a / some tests(s) for this - I can't see that the new code is currently used anywhere... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is looking good -- I have a few suggestions but I'm really happy with what this will look like
gusto/recovery/recovery_options.py
Outdated
@@ -0,0 +1,67 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add some docs for the module?
gusto/recovery/recovery_options.py
Outdated
DG_recovered_space = self.domain.spaces.H1 | ||
|
||
self.DG_options = RecoveryOptions(embedding_space=DG_embedding_space, | ||
recovered_space=DG_recovered_space) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also add the boundary method here, based on the domain?
gusto/recovery/recovery_options.py
Outdated
self.build_DG_options() | ||
self.build_HDiv_options() | ||
|
||
def build_DG_options(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be its own function? Or could it be part of the init method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have particularly strong feelings one way or another about this. I wrote it this way as I found it easier to keep track of the different options when coding it up. I'm happy for it to be in the ini
gusto/recovery/recovery_options.py
Outdated
DG_hori_ele = FiniteElement('DG', self.cell, 1, variant='equispaced') | ||
DG_vert_ele = FiniteElement('DG', interval, 2, variant='equispaced') | ||
CG_hori_ele = FiniteElement('CG', self.cell, 1) | ||
CG_vert_ele = FiniteElement('CG', interval, 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we always want this to be degree 2? Maybe we do for some cases but not all -- so I think we need an argument controlling which degrees we use for the theta space?
I think we could use this in some existing tests:
And if we want to be really thorough, we should add a unit-test specifically testing that the spaces set up have the appropriate degrees |
boundary_method=BoundaryMethod.taylor) | ||
theta_opts = RecoveryOptions(embedding_space=VDG1, | ||
recovered_space=VCG1) | ||
recovery_spaces = RecoverySpaces(domain) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boundary methods are required for this example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also want use_vector_spaces=True
# Transport schemes -- specify options for using recovery wrapper | ||
recovery_spaces = RecoverySpaces(domain) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boundary methods are required here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and also use_vector_spaces=True
gusto/recovery/recovery_options.py
Outdated
domain (:class:`Domain`): the model's domain object, containing the | ||
mesh and the compatible function spaces. | ||
|
||
boundary_method (:variable:'dict'): A dictionary containing the space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify that this is an optional argument and give its default value.
gusto/recovery/recovery_options.py
Outdated
mesh and the compatible function spaces. | ||
|
||
boundary_method (:variable:'dict'): A dictionary containing the space | ||
it applies to along with the chosen method. Acceptable keys are "DG", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the use_vector_spaces
argument? please document.
gusto/recovery/recovery_options.py
Outdated
project_low_method='project', | ||
broken_method='project', | ||
boundary_method=HDiv_boundary_method) | ||
# ---------------------------------------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment not in right place
@tommbendall do we need an integration test or example that sets up and runs a problem on mixed order spaces? |
We could add a travelling vortex to the examples? But I think that could also happen in a different PR |
gusto/recovery/recovery_options.py
Outdated
domain (:class:`Domain`): the model's domain object, containing the | ||
mesh and the compatible function spaces. | ||
|
||
boundary_method (:variable:'dict', optional: A dictionary containing the space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
close brackets, use consistent formatting for the keys, new sentence for the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might also need the other quotation marks, for consistency with the other comments and possibly for how the docs are formatted.
gusto/recovery/recovery_options.py
Outdated
the boundary method is to be applied to along with specified method. Acceptable keys are "DG", | ||
"HDiv" and theta, defaults to None | ||
|
||
use_vector_spaces (boolean, optional) Determins if we need to use DG / CG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling: determines and Hcurl, bool
rather than boolean
and put a colon after the type - necessary for consistency but also because this can affect how the docs look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very nearly there, there's still just a few things to tidy up in the docstrings
gusto/recovery/recovery_options.py
Outdated
""" | ||
Args: | ||
domain (:class:`Domain`): the model's domain object, containing the | ||
mesh and the compatible function spaces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you indent the continuation lines, so that it's clear which are the variables?
gusto/recovery/recovery_options.py
Outdated
@@ -0,0 +1,119 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could still do with a docstring at the top of the file describing what this module contains, e.g.
"""Contains an object to generate the set of spaces used for the recovery wrapper."""
gusto/recovery/recovery_options.py
Outdated
|
||
boundary_method (:variable:'dict', optional): A dictionary containing the space | ||
the boundary method is to be applied to along with specified method. Acceptable keys are "DG", | ||
"HDiv" and "theta". acceptable values are (BoundaryMetghod.taylor/hcurl/extruded), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"HDiv" and "theta". acceptable values are (BoundaryMetghod.taylor/hcurl/extruded), | |
"HDiv" and "theta". acceptable values are (BoundaryMethod.taylor/hcurl/extruded), |
gusto/recovery/recovery_options.py
Outdated
the boundary method is to be applied to along with specified method. Acceptable keys are "DG", | ||
"HDiv" and "theta". acceptable values are (BoundaryMetghod.taylor/hcurl/extruded), | ||
passed as ('space', 'boundary method') | ||
Defaults to None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Defaults to
should be on the same line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's one more typo to fix! And about the continuation lines -- documentation of arguments that go over one line need indenting also. I've put suggestions in
gusto/recovery/recovery_options.py
Outdated
""" | ||
Args: | ||
domain (:class:`Domain`): the model's domain object, containing the | ||
mesh and the compatible function spaces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mesh and the compatible function spaces. | |
mesh and the compatible function spaces. |
gusto/recovery/recovery_options.py
Outdated
mesh and the compatible function spaces. | ||
|
||
boundary_method (:variable:'dict', optional): A dictionary containing the space | ||
the boundary method is to be applied to along with specified method. Acceptable keys are "DG", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the boundary method is to be applied to along with specified method. Acceptable keys are "DG", | |
the boundary method is to be applied to along with specified method. Acceptable keys are "DG", |
gusto/recovery/recovery_options.py
Outdated
|
||
boundary_method (:variable:'dict', optional): A dictionary containing the space | ||
the boundary method is to be applied to along with specified method. Acceptable keys are "DG", | ||
"HDiv" and "theta". acceptable values are (BoundaryMethod.taylor/hcurl/extruded), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"HDiv" and "theta". acceptable values are (BoundaryMethod.taylor/hcurl/extruded), | |
"HDiv" and "theta". acceptable values are (BoundaryMethod.taylor/hcurl/extruded), |
gusto/recovery/recovery_options.py
Outdated
boundary_method (:variable:'dict', optional): A dictionary containing the space | ||
the boundary method is to be applied to along with specified method. Acceptable keys are "DG", | ||
"HDiv" and "theta". acceptable values are (BoundaryMethod.taylor/hcurl/extruded), | ||
passed as ('space', 'boundary method'). Defaults to None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
passed as ('space', 'boundary method'). Defaults to None | |
passed as ('space', 'boundary method'). Defaults to None |
gusto/recovery/recovery_options.py
Outdated
passed as ('space', 'boundary method'). Defaults to None | ||
|
||
use_vector_spaces (bool, optional):. Determines if we need to use DG / CG | ||
space for the embedded and recovery space for the HDiv field instead of the usual |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space for the embedded and recovery space for the HDiv field instead of the usual | |
space for the embedded and recovery space for the HDiv field instead of the usual |
gusto/recovery/recovery_options.py
Outdated
|
||
use_vector_spaces (bool, optional):. Determines if we need to use DG / CG | ||
space for the embedded and recovery space for the HDiv field instead of the usual | ||
HDiv, HCurl spaces. Defaults to False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HDiv, HCurl spaces. Defaults to False | |
HDiv, HCurl spaces. Defaults to False |
gusto/recovery/recovery_options.py
Outdated
@@ -0,0 +1,117 @@ | |||
"""Contains an obect to generate the set of spaces used for the recovery wrapper """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"""Contains an obect to generate the set of spaces used for the recovery wrapper """ | |
"""Contains an object to generate the set of spaces used for the recovery wrapper """ | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Dan! This looks great now
Code to build/find the necessary spaces for recovered transport for lowest order / mixed order finite element domains.
Spaces are found and build bespoke to the family of the de-Rham complex