Multiple volumes by majosm · Pull Request #726 · illinois-ceesd/mirgecom

majosm · 2022-08-15T19:42:04Z

Extracted out of #630. Depends on inducer/grudge#246 and inducer/meshmode#342.

Questions for the review @lukeolson:

inducer · 2022-08-15T20:15:45Z

-                          kappa=1., s0=-6., time=0, operator_states_quad=None,
-                          grad_cv=None, quadrature_tag=None, boundary_kwargs=None,
+                          kappa=1., s0=-6., time=0, quadrature_tag=DISCR_TAG_BASE,
+                          volume_dd=DD_VOLUME_ALL, boundary_kwargs=None,


Delete boundary_kwargs?

Can't yet, some of the drivers still use it (isolator and nozzle).

inducer · 2022-08-15T20:19:39Z

+                          kappa=1., s0=-6., time=0, quadrature_tag=DISCR_TAG_BASE,
+                          volume_dd=DD_VOLUME_ALL, boundary_kwargs=None,


quadrature_tag and volume_dd both contain a discretization tag.

You could do volume_dd=None and then allow (with a deprecation warning) a quadrature_tag to be passed, otherwise raise.

(Here and elsewhere.)

As we discussed elsewhere, the two discretization tags here are only sometimes redundant. I tried to remove those cases.

inducer · 2022-08-15T20:21:53Z

+    dd_base = volume_dd
+    dd_vol = dd_base.with_discr_tag(quadrature_tag)


I'm getting myself confused on how dd_base, dd_vol, volume_dd are intended to be different. (Add a comment?)

I did my best to clarify the naming. I'm now using dd for the function parameters, checking that they're volumes where required, and then using dd_vol and dd_vol_quad internally for the base and quadrature discretizations, respectfully (and using the same naming scheme for other dds like bdry/allfaces and arrays).

inducer · 2022-08-15T20:24:37Z

+        dd_allfaces = dd_trace.with_domain_tag(
+            replace(dd_trace.domain_tag, tag=FACE_RESTR_ALL))


This is clunky. 🤷

Fixed in inducer/grudge#280 with with_boundary_tag.

inducer · 2022-08-15T20:47:56Z

        discr, gas_model, boundaries, state, *, time=0.0,
        numerical_flux_func=num_flux_central,
-        quadrature_tag=DISCR_TAG_BASE,
+        quadrature_tag=DISCR_TAG_BASE, volume_dd=DD_VOLUME_ALL,


Extra quadrature_tag

Still needed.

inducer · 2022-08-15T20:48:29Z

        discr, gas_model, boundaries, state, *, time=0.0,
        numerical_flux_func=num_flux_central,
-        quadrature_tag=DISCR_TAG_BASE,
+        quadrature_tag=DISCR_TAG_BASE, volume_dd=DD_VOLUME_ALL,


Extra quadrature_tag

Still needed.

inducer · 2022-08-15T20:49:47Z

                inviscid_numerical_flux_func=inviscid_facial_flux_rusanov,
                gradient_numerical_flux_func=num_flux_central,
                viscous_numerical_flux_func=viscous_facial_flux_central,
-                quadrature_tag=DISCR_TAG_BASE, return_gradients=False,


Extra quadrature_tag

Still needed.

inducer · 2022-08-15T20:54:08Z

+            from meshmode.distributed import get_partition_by_pymetis
+            return get_partition_by_pymetis(mesh, num_ranks)
+
+    if comm.Get_rank() == 0:


This all looks pretty bureaucratic. Can we do this in grudge/meshmode? What's the background to this?

Removed from this PR (still need to figure out how to clean it up, but we'll worry about that later.)

inducer · 2022-08-15T20:54:26Z

        domain_boundary_states, grad_cv, interior_grad_cv_pairs,
-        grad_t, interior_grad_t_pairs, quadrature_tag=None,
-        numerical_flux_func=viscous_facial_flux_central, time=0.0):
+        grad_t, interior_grad_t_pairs, quadrature_tag=DISCR_TAG_BASE,


Extra quadrature_tag

Still needed.

majosm · 2022-09-01T19:51:46Z

@matthiasdiener @inducer Am I correct in guessing that the current MPI communication tagging strategy isn't guaranteed to work when you call the same operator multiple times in a single compiled function? I ran into some trouble with the example I just set up (multiple-volumes.py), which creates multiple copies of the same mesh with different initial conditions and calls euler_operator on each of them in the RHS. It worked fine in eager, but when I ran it in lazy it looked like the volumes were getting the wrong data at the partition boundaries. I "fixed" it with c0780b8, though I imagine that solution isn't going to work in general. 🙂

MTCam · 2022-09-01T20:01:01Z

@matthiasdiener @inducer Am I correct in guessing that the current MPI communication tagging strategy isn't guaranteed to work when you call the same operator multiple times in a single compiled function? (...)
I "fixed" it with c0780b8, though I imagine that solution isn't going to work in general. 🙂

I know I wasn't asked, but what about doing something like:

def euler_operator(..., cv_comm_tag=EulerCVCommTag, temperature_comm_tag=EulerTemperatureCommTag)

... after making those comm tags importable classes? That way the driver writer can override or pass different tags if needed/wanted?

majosm · 2022-09-01T20:19:55Z

I know I wasn't asked, but what about doing something like:

def euler_operator(..., cv_comm_tag=EulerCVCommTag, temperature_comm_tag=EulerTemperatureCommTag)

... after making those comm tags importable classes? That way the driver writer can override or pass different tags if needed/wanted?

I guess so, but it would be nice if we didn't have to do that...

That version would also leave me with some (admittedly self-inflicted) challenges, since I made the number of volumes in that example a parameter. 🙂

majosm · 2022-09-13T17:08:20Z

Ready for review again. I'm going to let the production CI fail until this gets pretty close to being ready to merge (and I think the doc CI will fail until inducer/grudge#280 goes in).

majosm · 2022-09-16T14:47:59Z

The doc CI failures are one thing getting in the way of merging this into main at the moment. @matthiasdiener and I looked at this a couple of weeks ago, and IIRC the conclusion was that it seems to be pulling doc references from grudge main, not the multi-volume branch. Maybe there's a way to work around this?

inducer · 2022-09-16T14:51:41Z

https://github.com/inducer/pytato/blob/69e2a6c29df691c529f55be44fab0d8f1d193c4c/doc/conf.py#L45-L49

lukeolson · 2022-09-16T16:53:52Z

+    make_fluid_state
+)
+from logpyle import IntervalTimer, set_dt
+from mirgecom.euler import extract_vars_for_logging


Should group this import above. There are a couple that need grouping.

inducer reviewed Aug 15, 2022

View reviewed changes

majosm force-pushed the multi-volume branch from 5a13d50 to cfad592 Compare August 25, 2022 20:32

majosm added 2 commits August 26, 2022 11:48

add multi-volume support

521388e

deprecate mpi_communicator argument in create_discretization_collection

d7f83ce

majosm force-pushed the multi-volume branch from 658319f to 85310b2 Compare August 29, 2022 17:20

Merge remote-tracking branch 'origin/main' into multi-volume

5988087

majosm force-pushed the multi-volume branch 2 times, most recently from 3109bca to faf3717 Compare August 29, 2022 17:29

majosm mentioned this pull request Aug 29, 2022

Multiple independent volumes inducer/grudge#280

Merged

majosm force-pushed the multi-volume branch from 9c8ebf0 to 64bf927 Compare August 30, 2022 16:47

majosm added 6 commits September 1, 2022 12:11

Merge remote-tracking branch 'origin/main' into multi-volume

53965f4

don't depend on full grudge multi-volume support yet

ed2be88

change grudge branch

191d7ca

use new DOFDesc methods

6b475f9

clean up some DOFDesc stuff

e861f11

tweak indepedent volumes test

85e8dcb

majosm force-pushed the multi-volume branch 2 times, most recently from 7200ebb to c0780b8 Compare September 1, 2022 17:57

majosm force-pushed the multi-volume branch from c0780b8 to d41c6d5 Compare September 2, 2022 18:44

majosm mentioned this pull request Sep 2, 2022

Add optional user-specified comm_tag to operators #741

Merged

8 tasks

Merge remote-tracking branch 'origin/main' into multi-volume

d3d42c8

majosm force-pushed the multi-volume branch 4 times, most recently from 5eaf339 to 87339e5 Compare September 7, 2022 22:45

majosm mentioned this pull request Sep 7, 2022

Refactor diffusion to look more like Euler/NS #744

Merged

16 tasks

Merge remote-tracking branch 'origin/main' into multi-volume

ec7a340

majosm added 7 commits September 12, 2022 22:50

add multiple independent volume example

07405f9

add missing btag -> bdtag promotion in euler_operator

a6940d2

unify dd naming

fd3c2c4

add missing dd argument to get_inviscid_timestep

ef3863e

remove deprecated +=

4aa83f1

add missing btag -> bdtag promotion in make_operator_fluid_states

8c7de8a

wrap btag -> bdtag promotion into a function

1d235ee

majosm force-pushed the multi-volume branch from 87339e5 to 1d235ee Compare September 13, 2022 03:51

majosm requested a review from inducer September 13, 2022 17:06

majosm marked this pull request as ready for review September 15, 2022 15:15

majosm requested a review from lukeolson September 16, 2022 14:07

fix doc build issue

3dfca1b

majosm force-pushed the multi-volume branch from 75aad74 to 3dfca1b Compare September 16, 2022 15:05

clarify what multiple-volumes example does

bbfbd77

lukeolson approved these changes Sep 16, 2022

View reviewed changes

majosm added 5 commits September 16, 2022 12:29

clarify solution setup in test_independent_volumes

17090ab

group euler imports

ab0ecc6

set grudge branch

0280f0d

Merge remote-tracking branch 'origin/main' into multi-volume

6cc3fb1

Merge remote-tracking branch 'origin/main' into multi-volume

401fd23

majosm merged commit 0a4a40a into illinois-ceesd:main Sep 21, 2022

majosm mentioned this pull request Sep 21, 2022

Multiphysics (production) #630

Closed

8 tasks

		kappa=1., s0=-6., time=0, quadrature_tag=DISCR_TAG_BASE,
		volume_dd=DD_VOLUME_ALL, boundary_kwargs=None,

		dd_base = volume_dd
		dd_vol = dd_base.with_discr_tag(quadrature_tag)

		dd_allfaces = dd_trace.with_domain_tag(
		replace(dd_trace.domain_tag, tag=FACE_RESTR_ALL))

Conversation

majosm commented Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

majosm commented Sep 1, 2022

Uh oh!

MTCam commented Sep 1, 2022

Uh oh!

majosm commented Sep 1, 2022

Uh oh!

majosm commented Sep 13, 2022

Uh oh!

majosm commented Sep 16, 2022

Uh oh!

inducer commented Sep 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

majosm commented Aug 15, 2022 •

edited

Loading