Wrapper of options #471

ta440 · 2024-01-09T04:07:16Z

This adds the ability to apply the different wrapper options of EmbeddedDG and Recovery to different tracers/prognostic variables in the transport step. This will enable tracers to live in different spaces and will fix a current compatibility issue where a DG and theta tracer can't both be used within the CoupledTransportEquation with respective sublimiters, as the ThetaLimiter requires the use of EmbeddedDG.

This is implemented by defining a new dictionary wrapper 'MixedOptions' which will store the individual wrapper options of the transported variables. Tracers have different options, or may have none defined at all. Currently, SUPG has not been implemented as an option for a tracer.

A new test file, 'test_mixed_fs_options.py' is made to test combinations of these different options for two tracers solved with a CoupledTransportEquation. Limiters are also used on all these tests. As this new script includes the DG1-DG1_equispaced combination, this test has been removed from the 'test_limiters.py' file.

…ions

tommbendall

Thanks very much for doing this Tim, I think this will enable us to lots of exciting things!

I think the methods for the MixedOptions wrapper look perfect and I think it's the right thing to have a new test, which also looks good.

However (as I've written in a detailed comment) I think we actually might want to separate the new object into two objects:

MixedFSOptions to live in configuration.py
MixedFSWrapper to live in wrappers.py and would be very close to what you've implemented already

The reason for this is that it keeps the code structure used for the existing Options/Wrappers.

My other thought (as I've written in another comment) is about whether we can avoid having the mixed_options property for wrapper objects?

I'm very happy to discuss these things offline.

tommbendall · 2024-01-30T16:54:32Z

gusto/time_discretisation.py

-                self.wrapper = RecoveryWrapper(self, options)
-            elif self.wrapper_name == "supg":
-                self.wrapper = SUPGWrapper(self, options)
+            if type(options) == MixedOptions:


Suggested change

if type(options) == MixedOptions:

if type(options) is MixedOptions:

I think when type checking, it's slightly better to use is rather than ==

tommbendall · 2024-01-30T17:08:00Z

gusto/time_discretisation.py

-            elif self.wrapper_name == "supg":
-                self.wrapper = SUPGWrapper(self, options)
+            if type(options) == MixedOptions:
+                self.wrapper = options


Not that what you've done isn't a sensible way of implementing this, but it's occurred to me that it isn't quite consistent with how the existing wrappers work.

At the moment we have two broad types of objects: Options and Wrappers, and I think what we've done here is mix up the two a bit.

So now I'm thinking the best way of mirroring this structure for mixed function spaces would be to have:

MixedFSOptions which is defined in configuration.py and is essentially a glorified dictionary

MixedFSWrapper which is defined in wrappers.py and has the same methods as other Wrapper objects

tommbendall · 2024-01-30T17:10:33Z

gusto/wrappers.py

        assert isinstance(self.options, RecoveryOptions), \
-            'Embedded DG wrapper can only be used with Recovery Options'
+            'Recovery wrapper can only be used with Recovery Options'


Thanks for correcting this!

tommbendall · 2024-01-30T17:11:28Z

gusto/wrappers.py

+            else:
+                x_in_sub.assign(field)
+
+    def post_apply(self, x_out):


This method looks exactly right to me

tommbendall · 2024-01-30T17:11:45Z

gusto/wrappers.py

+        self.x_out = Function(self.function_space)
+
+    def pre_apply(self, x_in):
+        """


This method looks exactly right to me

tommbendall · 2024-01-30T17:34:06Z

gusto/wrappers.py

@@ -33,6 +33,9 @@ def __init__(self, time_discretisation, wrapper_options):
        self.time_discretisation = time_discretisation
        self.options = wrapper_options
        self.solver_parameters = None
+        self.idx = None
+        self.mixed_options = False


I'm a little bit uncertain about having a general Wrapper object know about mixed_options...

It feels like the inheritance of properties could be the wrong way around, as a MixedFSWrapper has several subwrappers, but the subwrappers then need to know that they are subwrapper. Ideally it would be better if they don't know that they are subwrappers!

Looking below at why you've had to do this, it's to set the function space. I'm just wondering if we can find a cleaner way to set original_space? Maybe idx could be an argument to the __init__ or setup method of the base Wrapper class?

tommbendall

Thanks again for this Tim. I think we're nearly there.

I understand why you needed to introduce the tracer_fs to the other wrappers. I think we could go a bit further with this by:

renaming it as original_space (since it might not always be used with tracers!)
having it set more explicitly through the setup method, which will help make the code clearer

tommbendall · 2024-02-27T11:24:10Z

gusto/wrappers.py

@@ -161,13 +169,19 @@ class RecoveryWrapper(Wrapper):
    def setup(self):
        """Sets up function spaces and fields needed for this wrapper."""

+        print(self.options)


This just needs removing!

tommbendall · 2024-02-27T11:26:54Z

gusto/wrappers.py

@@ -33,6 +33,7 @@ def __init__(self, time_discretisation, wrapper_options):
        self.time_discretisation = time_discretisation
        self.options = wrapper_options
        self.solver_parameters = None
+        self.tracer_fs = None


Suggested change

self.tracer_fs = None

self.original_space = None

tommbendall · 2024-02-27T11:27:37Z

gusto/time_discretisation.py


-            self.residual = self.wrapper.label_terms(self.residual)
+                    subwrapper.setup()


Suggested change

subwrapper.setup()

subwrapper.setup(self.equation.spaces[field_idx])

tommbendall · 2024-02-27T11:27:50Z

gusto/time_discretisation.py

+                    field_idx = equation.field_names.index(field)
+
+                    # Store the original space of the tracer
+                    subwrapper.tracer_fs = self.equation.spaces[field_idx]


Suggested change

subwrapper.tracer_fs = self.equation.spaces[field_idx]

tommbendall · 2024-02-27T11:29:06Z

gusto/time_discretisation.py

+                    map_if_true=replace_test_function(new_test_mixed))
+
+            else:
+                self.wrapper.setup()


Suggested change

self.wrapper.setup()

self.wrapper.setup(self.fs)

I think this should be self.fs but I'm not sure...? Maybe it is just None

I think we would want this to be None, as we want self.original_space=None when not using the mixed wrapper.

tommbendall · 2024-02-27T11:32:36Z

gusto/wrappers.py

+            original_space = self.tracer_fs
+        else:
+            original_space = self.time_discretisation.fs
+


Again this snippet changes to

self.original_space = original_space

tommbendall · 2024-02-27T11:32:48Z

gusto/wrappers.py

@@ -184,10 +198,17 @@ def setup(self):
        # Internal variables to be used
        # -------------------------------------------------------------------- #

-        self.x_in_tmp = Function(self.time_discretisation.fs)
+        if self.tracer_fs is not None:


Suggested change

if self.tracer_fs is not None:

if self.original_space is not None:

tommbendall · 2024-02-27T11:33:00Z

gusto/wrappers.py

@@ -184,10 +198,17 @@ def setup(self):
        # Internal variables to be used
        # -------------------------------------------------------------------- #

-        self.x_in_tmp = Function(self.time_discretisation.fs)
+        if self.tracer_fs is not None:
+            self.x_in_tmp = Function(self.tracer_fs)


Suggested change

self.x_in_tmp = Function(self.tracer_fs)

self.x_in_tmp = Function(self.original_space)

tommbendall · 2024-02-27T11:34:03Z

gusto/wrappers.py

        self.x_in = Function(self.function_space)
        self.x_out = Function(self.function_space)
-        if self.time_discretisation.idx is None:
+
+        if self.tracer_fs is not None:


Suggested change

if self.tracer_fs is not None:

if self.original_space is not None:

tommbendall · 2024-02-27T11:34:12Z

gusto/wrappers.py

-        if self.time_discretisation.idx is None:
+
+        if self.tracer_fs is not None:
+            self.x_projected = Function(self.tracer_fs)


Suggested change

self.x_projected = Function(self.tracer_fs)

self.x_projected = Function(self.original_space)

ta440 · 2024-02-28T03:28:43Z

Thanks for the comments, Tom!
By having the original_space set in the setup, I assume you mean for the individual wrappers (EmbeddedDG, Recovery), as if it was in the original wrapper object, we still have to pass original space as an argument into the individual wrapper instances. If so, I don't think we need original_space to actually be a property of wrappers, as we pass it in if using MixedFSWrapper, and it will default to None if not.
I think this requires the argument to the setup() function to be called something other than original_space, to avoid confusion when we define it later on. I've put in previous_space as a placeholder, but can change it to a better name?
Please let me know if this wasn't the implementation you were thinking of! Perhaps there is a simpler way to set self.original_space before performing the subwrapper.setup()?

tommbendall

Thanks Tim for doing all of this! I think this is ready now.

ta440 added 16 commits December 20, 2023 02:59

started MixedOptions and an integration test for this

573ad52

MixedOptions dictionary added

e04c73a

improvements to MixedOptions

4a13df3

more mixed options changes

6165057

more changes to mixed options

775d3b2

working on new test functions with mixed options

793768e

more changes to test functions for multiple wrappers

5f05367

more changes to mixed options with replacing test functions

5c13db1

mixed options works for embedded dg and recovery

806a49e

moved DG1-DG1 equispaced test from test_limiters to test_mixed_fs_opt…

2be1354

…ions

finalising mixed options test script

6d2fc9b

lint

25ed243

lint

f410d2c

lint

d166a22

lint

aa3d7c0

lint

f1e958a

ta440 marked this pull request as ready for review January 23, 2024 01:16

remove dubugging statements

51f6f7b

tommbendall requested changes Jan 30, 2024

View reviewed changes

ta440 added 4 commits February 20, 2024 02:40

separate MixedFSOptions and MixedFSWrapper to align with exising code

55e0856

lint

164041c

lint

edcc929

lint

b317f39

tommbendall requested changes Feb 27, 2024

View reviewed changes

set up original spaces for MixedFSWrapper within the subwrappers

0700381

ta440 added 3 commits February 28, 2024 19:30

make original_space a base wrapper property

2f517e1

make original_space a required argument for wrapper.setup()

5c2980d

lint

222c396

tommbendall approved these changes Mar 1, 2024

View reviewed changes

Merge branch 'main' into wrapper_of_options

66ce750

tommbendall merged commit 6d76ca7 into main Mar 1, 2024
4 checks passed

tommbendall deleted the wrapper_of_options branch March 1, 2024 10:33

tommbendall mentioned this pull request Mar 13, 2024

Can't use wrapper or limiter options with physics yet #419

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrapper of options #471

Wrapper of options #471

ta440 commented Jan 9, 2024 •

edited

Loading

tommbendall left a comment

tommbendall Jan 30, 2024

tommbendall Jan 30, 2024

tommbendall Jan 30, 2024

tommbendall Jan 30, 2024

tommbendall Jan 30, 2024

tommbendall Jan 30, 2024

tommbendall left a comment

tommbendall Feb 27, 2024

tommbendall Feb 27, 2024

tommbendall Feb 27, 2024

tommbendall Feb 27, 2024

tommbendall Feb 27, 2024

ta440 Feb 28, 2024 •

edited

Loading

tommbendall Feb 27, 2024

tommbendall Feb 27, 2024

tommbendall Feb 27, 2024

tommbendall Feb 27, 2024

tommbendall Feb 27, 2024

ta440 commented Feb 28, 2024 •

edited

Loading

tommbendall left a comment

	if type(options) == MixedOptions:
	if type(options) is MixedOptions:


		self.residual = self.wrapper.label_terms(self.residual)
		subwrapper.setup()

	subwrapper.setup()
	subwrapper.setup(self.equation.spaces[field_idx])

	if self.tracer_fs is not None:
	if self.original_space is not None:

	self.x_in_tmp = Function(self.tracer_fs)
	self.x_in_tmp = Function(self.original_space)

	self.x_projected = Function(self.tracer_fs)
	self.x_projected = Function(self.original_space)

Wrapper of options #471

Wrapper of options #471

Conversation

ta440 commented Jan 9, 2024 • edited Loading

tommbendall 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

tommbendall 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

ta440 Feb 28, 2024 • edited Loading

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

ta440 commented Feb 28, 2024 • edited Loading

tommbendall left a comment

Choose a reason for hiding this comment

ta440 commented Jan 9, 2024 •

edited

Loading

ta440 Feb 28, 2024 •

edited

Loading

ta440 commented Feb 28, 2024 •

edited

Loading