Coupled #41

ZoeLeibowitz · 2025-02-26T18:36:47Z

New machinery to solve for more than 1 field within a single matrix system.

TODOs:

Add more extensive tests
Utilise some of the new machinery to support Tensor Functions
Start wiki page/project board to track current implementation status

JDBetteridge

There's a lot of code here most of which looks good, but I have two major concerns:

I'm not sure that the (existing) tests include sufficient coverage for the new code being added.
There isn't much in the way of documentation.
Perhaps there needs to be a more general project page to cover integration?

devito/petsc/iet/passes.py

JDBetteridge · 2025-03-06T12:14:25Z

devito/petsc/iet/passes.py

+    def _callback_builder(self):
+        sobjs = self.objbuilder.solver_objs
+        args = (self.injectsolve, self.objs, sobjs, self.timedep)
+        return (
+            CCBBuilder(*args, **self.kwargs)
+            if self.coupled else
+            CBBuilder(*args, **self.kwargs)
        )

-        # Solver setup
-        self.solversetup = BaseSetup(
-            self.solver_objs, objs, injectsolve, self.cbbuilder
+    def _setup(self):
+        sobjs = self.objbuilder.solver_objs
+        args = (self.injectsolve, self.objs, sobjs, self.cbbuilder)
+        return (
+            CoupledSetup(*args, **self.kwargs)
+            if self.coupled else
+            BaseSetup(*args, **self.kwargs)
        )

-        # Execute the solver
-        self.solve = Solver(
-            self.solver_objs, objs, injectsolve, iters,
-            self.cbbuilder, timedep=self.timedep
+    def _solver_execution(self):
+        sobjs = self.objbuilder.solver_objs
+        args = (
+            self.injectsolve, self.objs, sobjs,
+            self.iters, self.cbbuilder, self.timedep
+        )
+        return (
+            CoupledSolver(*args)
+            if self.coupled else
+            Solver(*args)
        )


There's a lot of code repetition in these methods, can it be factored out?

devito/petsc/iet/passes.py

JDBetteridge · 2025-03-06T12:24:12Z

devito/petsc/types/object.py

    @property
    def _C_free_priority(self):
-        return 3
+        return 4


🤨

https://xkcd.com/221/

yes, we need an enumeration or something, because what started off as a simple thing is now growing out of hand :)

devito/petsc/utils.py

FabioLuporini · 2025-03-07T08:16:59Z

devito/ir/iet/visitors.py

            if not issubclass(ctype, ctypes.Structure):
                return None
        except TypeError:
            # E.g., `ctype` is of type `dtypes_lowering.CustomDtype`


this comment to be moved inside the else?

FabioLuporini · 2025-03-07T08:17:22Z

devito/ir/iet/visitors.py

            # E.g., `ctype` is of type `dtypes_lowering.CustomDtype`
-            return None
+            if isinstance(obj, LocalCompositeObject):
+                ctype = obj


how can the ctype being a LocalCompositeObject? I struggle to follow here

FabioLuporini · 2025-03-07T08:18:47Z

devito/ir/iet/nodes.py

            ret.extend(self.pointer.free_symbols)
        else:
-            assert issubclass(self.pointer._C_ctype, ctypes._Pointer)
+            assert isinstance(self.pointer, LocalCompositeObject) or \


nitpicking, for code homogeneity : for multi-line statements, instead of the backslash, we use parenthesis for perfectly aligned indentation

assert (isinstance(....) or issubclass(...))

FabioLuporini · 2025-03-07T08:19:59Z

devito/operator/operator.py

        """
        # Create a symbol registry
        kwargs.setdefault('sregistry', SymbolRegistry())
+        # Update based on changes in #2509


We may need a more useful comment here

FabioLuporini · 2025-03-07T08:33:00Z

devito/petsc/types/object.py

    @property
    def _C_free_priority(self):
-        return 3
+        return 4


yes, we need an enumeration or something, because what started off as a simple thing is now growing out of hand :)

FabioLuporini · 2025-03-07T08:34:57Z

devito/tools/dtypes_lowering.py

    __str__ = __repr__


+class CustomIntType(CustomDtype):


Why a subclass and not just a specific instance?

FabioLuporini · 2025-03-07T08:36:18Z

devito/symbolics/extended_sympy.py

-                raise ValueError("Symbolic RHS `%s` must be of type `int`, found "
-                                 "`%s` instead" % (rhs, rhs.dtype))
+
+            is_int_type = isinstance(rhs.dtype, type) and \


maybe this should be moved inside a utility function?

To add: convention is to use parenthesis

FabioLuporini · 2025-03-07T08:36:57Z

devito/symbolics/extended_sympy.py

+
+    is_commutative = True
+
+    # Set the operator priority higher than SymPy (10.0) to force the overridden


can you elaborate what the issue is?

EdCaunt · 2025-03-10T11:25:31Z

devito/petsc/iet/passes.py

    # TODO: Are users going to be able to use PETSc command line arguments?
    # In firedrake, they have an options_prefix for each solver, enabling the use
    # of command line options
+    Null = objs['Null']


Is there any chance of this creating a KeyError? Ditto for two lines below

EdCaunt · 2025-03-10T11:30:41Z

devito/petsc/iet/passes.py

+            **self.kwargs
+        }
+
+        self.objbuilder = self._object_builder()


Could this just be a cached property rather than requiring a method? Ditto with some of the similar constructions further down in this init, which would reduce the number of lines of code.

EdCaunt · 2025-03-10T11:37:41Z

devito/petsc/iet/passes.py

+        self.args['timedep'] = self.timedep
+
+        self.cbbuilder = self._callback_builder()
+        self.args['cbbuilder'] = self.cbbuilder


Are any of these functions sensitive to the contents of self.args? I.e. do they need to be called in a specific order?

EdCaunt · 2025-03-10T11:38:18Z

devito/petsc/iet/passes.py

-        self.objbuilder = BaseObjectBuilder(injectsolve, **kwargs)
-        self.solver_objs = self.objbuilder.solver_objs
+    def _callback_builder(self):
+        return (CCBBuilder(**self.args) if self.coupled else CBBuilder(**self.args))


Nitpick: unnecessary brackets? (Ditto for the next few methods)

EdCaunt · 2025-03-10T11:39:33Z

devito/petsc/iet/passes.py

+def populate_matrix_context(efuncs, objs):
+    name = 'PopulateMatContext'
+
+    if name not in efuncs:


This hardcoded string check doesn't look great, is there no way to check for a class?

EdCaunt · 2025-03-10T13:55:08Z

tests/test_petsc.py

-        ' + x_matvec_f[x + 2, y + 1]/lctx->h_y**2' + \
-        ' - 2.0*x_matvec_f[x + 2, y + 2]/lctx->h_y**2' + \
-        ' + x_matvec_f[x + 2, y + 3]/lctx->h_y**2'
+        'x_f[x + 1, y + 2]/ctx0->h_x**2' + \


Nitpick: You can use parenthesis to join strings for tidiness

EdCaunt · 2025-03-10T13:58:01Z

tests/test_petsc.py

+        # Non-coupled
+        petsc1 = PETScSolve(eq1, target=e)
+        petsc2 = PETScSolve(eq2, target=g)
+        with switchconfig(openmp=False, mpi=True):


Switchconfig for MPI shouldn't be needed due to the parallel tag on the test

EdCaunt · 2025-03-10T13:58:28Z

tests/test_petsc.py

+        enorm1 = norm(e)
+        gnorm1 = norm(g)
+
+        # reset


Nitpick: start comment with uppercase

EdCaunt · 2025-03-10T14:00:15Z

tests/test_petsc.py

+
+    @skipif('petsc')
+    def test_coupled_structs(self):
+        grid = Grid(shape=(11, 11))


This gets used in a lot of tests

grid = Grid(shape=(11, 11)) e = Function(name='e', grid=grid, space_order=2) f = Function(name='f', grid=grid, space_order=2) g = Function(name='g', grid=grid, space_order=2) h = Function(name='h', grid=grid, space_order=2)

would a utility function to set these up be worthwhile?

Or a list comprehension?

EdCaunt · 2025-03-10T14:00:44Z

tests/test_petsc.py

+        ccode, hcode = op.cinterface(force=True)
+
+        dirname = op._compiler.get_jit_dir()
+        assert os.path.isfile(os.path.join(dirname, "%s.c" % name))


Fstring (see a couple of other places in this test)

* compiler/dsl: Add machinery to support coupled solvers

JDBetteridge self-requested a review March 5, 2025 17:38

JDBetteridge reviewed Mar 6, 2025

View reviewed changes

ZoeLeibowitz force-pushed the coupled-general branch from f696edb to 592d61d Compare March 6, 2025 16:45

ZoeLeibowitz requested review from EdCaunt, FabioLuporini, georgebisbas and mloubout March 6, 2025 18:41

ZoeLeibowitz marked this pull request as ready for review March 6, 2025 18:41

FabioLuporini reviewed Mar 7, 2025

View reviewed changes

EdCaunt reviewed Mar 10, 2025

View reviewed changes

ZoeLeibowitz added this to PETSc Mar 12, 2025

ZoeLeibowitz moved this to In Progress in PETSc Mar 12, 2025

ZoeLeibowitz added the enhancement New feature or request label Mar 12, 2025

ZoeLeibowitz force-pushed the coupled-general branch from 42331dc to daa4312 Compare March 18, 2025 18:55

compiler/dsl: Add machinery to support coupled solvers

0939e24

ZoeLeibowitz force-pushed the coupled-general branch from 7870990 to 0939e24 Compare March 19, 2025 11:29

ZoeLeibowitz added 2 commits March 19, 2025 15:51

tests: Add mpiexec to petsc pytest

30513ed

tests: Add -n 1 to pytest petsc

6b27b4b

ZoeLeibowitz merged commit c68dd4f into master Mar 19, 2025
29 checks passed

github-project-automation bot moved this from In Progress to Done in PETSc Mar 19, 2025

ZoeLeibowitz added a commit that referenced this pull request Mar 21, 2025

Coupled (#41)

7160249

* compiler/dsl: Add machinery to support coupled solvers

ZoeLeibowitz added a commit that referenced this pull request Mar 24, 2025

Coupled (#41)

0af998d

* compiler/dsl: Add machinery to support coupled solvers


		is_commutative = True

		# Set the operator priority higher than SymPy (10.0) to force the overridden

Coupled #41

Coupled #41

Uh oh!

Conversation

ZoeLeibowitz commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JDBetteridge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ZoeLeibowitz commented Feb 26, 2025 •

edited

Loading