Refactor DiscretizationCollection constructor #122

thomasgibson · 2021-06-07T01:03:47Z

This PR removes all the work that the current constructor for DiscretizationCollection is doing for setup. The proposed interface for creating these discretization collections is provided by the function make_discretization_collection.

grudge/discretization.py

inducer

Thanks for starting to think about this. Some thoughts from a quick scroll.

grudge/discretization.py

inducer · 2021-07-16T13:40:13Z

Is this ready for review from your end? (I ask because of the ambiguous state: no review requested, work still ongoing, but also not marked as draft)

inducer

Thanks! A few comments below.

grudge/discretization.py

inducer · 2021-07-16T17:32:18Z

grudge/discretization.py

+            from a :class:`grudge.dof_desc.DOFDesc`
+            to a :class:`meshmode.discretization.Discretization`.
+            At minimum, this should include a base discretization given by
+            the dof descriptor :class:`grudge.dof_desc.DD_VOLUME`.


I think the documentation of what this is needs to be expanded a bit, to avoid some misunderstandings that I had when first reading this, mainly on how this relates to the discr_from_dd method. (Maybe a different name would also help... vol_discr_from_dd?)

The discr_from_dd method can be used to retrieve all sorts of stuff (boundary discretizations and whatnot). This discr_from_dd dictionary cannot be used to supply "pre-made" versions of those. (It might be attractive to allow that? not sure) Said another way, this should set an expectation for what can and cannot be part of this mapping.

This is treated as read-only, i.e. additional discretizations made by discr_from_dd aren't added to this.

I decided to do away with the redundant discretizations altogether and streamline them through to discr_from_dd where possible: ae3a105.

I personally like this a lot more

I haven’t been following this PR closely, so I’m way out of the loop on what exactly is being changed… But I just wanted to ask: what will happen here when we add multiple volume discretizations? Is the _base_discr change going to be compatible with that?

The _base_discr is specifying the "base"/interpolatory element group on the mesh. So one question I have: If you have multiple volume discretizations, will they have different meshes? If so, then _base_discr should probably be a set of "base"/interpolator discretization objects defined on their appropriate volume meshes.

@majosm When you are at a point where you are trying to couple different discretizations (I think that's going to be MIRGECom + MIRGEHeat right?), I would appreciate being involved at some point in the discussion so I can see what's needed in grudge.

The _base_discr is specifying the "base"/interpolatory element group on the mesh. So one question I have: If you have multiple volume discretizations, will they have different meshes? If so, then _base_discr should probably be a set of "base"/interpolator discretization objects defined on their appropriate volume meshes.

I think there will need to be a global mesh (so we have connectivity between the different volumes), but it will get split into sub-meshes for each volume in order to construct the discretizations for each.

Ah okay! This makes sense. Then it sounds like what we want to make sure is that we pass in some mapping to make_discretization_collection (I proposed doing this as some kwarg for metadata) that associates sub-meshes with "base" (often referred to as the volume discretization) discretizations. I would be curious to know your opinion on the matter discussed here: #122 (comment)

grudge/discretization.py

inducer · 2021-07-16T17:36:18Z

grudge/discretization.py

+                 discr_tag_to_group_factory,
+                 discr_from_dd,
+                 dist_boundary_connections,
+                 mpi_communicator=None):
        """


This version is definitely no longer compatible with code that calls the DiscretizationCollection constructor directly. That's OK for grudge because you've fixed all the call sites, and it's OK for mirgecom because it only uses EagerDiscretization to which you've added the backward compatibility code. But I'm not sure it's OK for all other users? (cc @alexfikl)

Alternatively, the compat code could live here.

I have some grudge code sitting around, but it needs a lot of changes to move away from the deprecated symbolic stuff, so I'm fine with making a more breaking change here.

Afraid I don't really know a way around this without resorting to some hacky python. I'd prefer not to make this overly complicated if possible. How about once this PR is merged, we pin an issue describing how the interface has changed?

inducer · 2021-07-16T17:38:18Z

grudge/discretization.py

+    :arg discr_context: A :class:`~meshmode.mesh.Mesh` object to define
+        the discretization collection over.


Not sure I love the name. In pytential, @alexfikl used "places": https://documen.tician.de/pytential/symbolic.html in a similar role.

In particular, the GeometryCollection class in there. (The name was used in the code before introducing the GeometryCollection, so I just named it like that to stay consistent)

To be honest, I'm starting to like this placeholder business less and less the more I think about it. make_discretization_collection is intended to be the primary user-facing way to create the DiscretizationCollection And I can already imagine having an ambiguous placeholder argument causing confusion later on down the line.

I am not a fan, and I think we should avoid them if we can. I would rather just rename discr_context back to mesh, document it as such, and add an optional kwarg for discretization-related metadata that can be leveraged in the future. Thoughts @inducer @alexfikl ?

@majosm This is probably a relevant discussion.

I'm starting to like this placeholder business less and less the more I think about it.

Not sure I understand what you're objecting to. Having symbolic names for parts of the domain? Something else? Sorry if I'm being dense.

Sorry if I wasn't very clear. I was referring to using placeholder arguments in make_discretization_collection like places or discr_context (determined by position alone) which can be pretty much anything (mesh, discretizations, dicts of discretizations). I don't feel comfortable doing that.

I would rather try to use something along the lines of **kwargs, which is a bit more controlled from my perspective.

Personally, I feel like I've had OK experiences with those types of arguments, if used judiciously. Examples:

loopy functions will often take objects or string representations that parse to them. (e.g. domains, statements)

pyopencl.Array's constructor can take a context or an array as first argument.

The type of an object can be unambiguously determined, so I don't see a big risk in taking e.g. mesh as a shorthand for {DEFAULT_VOLUME: mesh}.

On the other hand, I find **kwargs parsing kind of unattractive:

Forget to check for extra arguments

Assuming you mean we would use mesh= or geometries= as alternatives to each other, we'd need to add logic to reject calls that have both.

**kwargs have to be the last argument. If that makes the interface kwargs-only, then all calls become kind of wordy, which isn't amazing for high-profile API real estate like the effective discretization collection constructor.

inducer · 2021-07-16T17:39:17Z

grudge/discretization.py

+        the discretization collection over.
+    :arg discr_tag_to_group_factory: A mapping from discretization tags
+        (typically one of: :class:`grudge.dof_desc.DISCR_TAG_BASE`,
+        :class:`grudge.dof_desc.DISCR_TAG_MODAL`, or


Is overriding the meaning of "modal" allowed?

I think this is addressed in 23dfa75. A default modal factory is provided whenever it is not explicitly provided by the user (in a similar way to how there is a default simplex group factor)

inducer · 2021-07-16T17:40:21Z

grudge/discretization.py

+    # Modal discr should always comes from the base discretization
+    discr_tag_to_group_factory[DISCR_TAG_MODAL] = \
+        _generate_modal_group_factory(
+            discr_tag_to_group_factory[DISCR_TAG_BASE]
+        )


This should error if DISCR_TAG_MODAL was supplied, instead of silently overwriting. The docs specifically allow supplying modal.

I've updated this in 23dfa75; I've opted to only provide the group factor when it's not provided by the user

grudge/discretization.py

inducer · 2021-07-16T17:44:44Z

Unsubscribing... @-mention or request review once it's ready for a look or needs attention.

inducer · 2022-03-17T05:20:52Z

Superseded by #236.

thomasgibson marked this pull request as draft June 7, 2021 01:03

thomasgibson linked an issue Jun 7, 2021 that may be closed by this pull request

DiscretizationCollection constructor should do no work #104

Open

thomasgibson force-pushed the dcoll-refactor branch from 735eb77 to 1b8153d Compare June 7, 2021 01:05

inducer reviewed Jun 7, 2021

View reviewed changes

grudge/discretization.py Outdated Show resolved Hide resolved

inducer reviewed Jun 7, 2021

View reviewed changes

grudge/discretization.py Outdated Show resolved Hide resolved

grudge/discretization.py Outdated Show resolved Hide resolved

thomasgibson added the design decisions Anything related to overhauling or redesigning code label Jun 7, 2021

thomasgibson force-pushed the dcoll-refactor branch from e6de687 to ac5e7bf Compare June 17, 2021 17:34

thomasgibson marked this pull request as ready for review June 17, 2021 17:59

thomasgibson force-pushed the dcoll-refactor branch from 96d06fa to a71f6e8 Compare June 17, 2021 18:06

thomasgibson added 7 commits July 15, 2021 10:46

Refactor DiscretizationCollection constructor

56cb4bf

Use make_discretization_collection

37571ca

Remove DGDiscretizationWithBoundaries

36ab939

Pass a map from dof desc to discretization

7f1ad0c

Dont reference dcoll._volume_discr

49ddc83

Section off construction routines and fix dt utils test

8e2ea22

Clarify documentation

0f4a6e5

thomasgibson force-pushed the dcoll-refactor branch from eaf89ad to 0f4a6e5 Compare July 15, 2021 15:55

thomasgibson added 2 commits July 15, 2021 11:31

Update dcoll-generator function arguments

74eb9bb

Fix reductions tests

ec76b25

thomasgibson requested a review from inducer July 16, 2021 15:56

inducer reviewed Jul 16, 2021

View reviewed changes

alexfikl mentioned this pull request Jul 16, 2021

Improve GeometryCollection docs inducer/pytential#106

Merged

thomasgibson self-assigned this Sep 20, 2021

thomasgibson added 3 commits September 21, 2021 13:24

Merge remote-tracking branch 'origin/main' into dcoll-refactor

2ab0de5

Update hello-grudge.py to use the new dcoll constructor

3879e80

Remove references to _volum_discr

6bdc25c

thomasgibson force-pushed the dcoll-refactor branch from 22fdd11 to 6bdc25c Compare September 22, 2021 00:17

Remove eager construction of the modal discretization

23dfa75

thomasgibson force-pushed the dcoll-refactor branch from fa9a8b9 to 23dfa75 Compare September 22, 2021 00:29

Streamline discr fetching through to discr_from_dd

ae3a105

thomasgibson force-pushed the dcoll-refactor branch from 81b52e7 to ae3a105 Compare September 22, 2021 01:28

Minor doc edits

6b8458c

thomasgibson mentioned this pull request Sep 22, 2021

Modernize Mirgecom illinois-ceesd/mirgecom#360

Closed

inducer closed this Mar 17, 2022

inducer deleted the dcoll-refactor branch March 17, 2022 05:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor DiscretizationCollection constructor #122

Refactor DiscretizationCollection constructor #122

thomasgibson commented Jun 7, 2021 •

edited

Loading

inducer left a comment

inducer commented Jul 16, 2021

inducer left a comment

inducer Jul 16, 2021

thomasgibson Sep 22, 2021

majosm Sep 22, 2021

thomasgibson Sep 22, 2021

thomasgibson Sep 22, 2021 •

edited

Loading

majosm Sep 22, 2021

thomasgibson Sep 22, 2021

inducer Jul 16, 2021

alexfikl Jul 16, 2021

thomasgibson Sep 22, 2021

inducer Jul 16, 2021

alexfikl Jul 16, 2021

thomasgibson Sep 22, 2021 •

edited

Loading

thomasgibson Sep 22, 2021

inducer Sep 23, 2021

thomasgibson Sep 23, 2021

thomasgibson Sep 23, 2021

inducer Sep 25, 2021

inducer Jul 16, 2021

thomasgibson Sep 22, 2021 •

edited

Loading

inducer Jul 16, 2021

thomasgibson Sep 22, 2021 •

edited

Loading

inducer commented Jul 16, 2021

inducer commented Mar 17, 2022

		:arg discr_context: A :class:`~meshmode.mesh.Mesh` object to define
		the discretization collection over.

Refactor DiscretizationCollection constructor #122

Refactor DiscretizationCollection constructor #122

Conversation

thomasgibson commented Jun 7, 2021 • edited Loading

inducer left a comment

Choose a reason for hiding this comment

inducer commented Jul 16, 2021

inducer 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

thomasgibson Sep 22, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasgibson Sep 22, 2021 • 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

Choose a reason for hiding this comment

thomasgibson Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasgibson Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

inducer commented Jul 16, 2021

inducer commented Mar 17, 2022

thomasgibson commented Jun 7, 2021 •

edited

Loading

thomasgibson Sep 22, 2021 •

edited

Loading

thomasgibson Sep 22, 2021 •

edited

Loading

thomasgibson Sep 22, 2021 •

edited

Loading

thomasgibson Sep 22, 2021 •

edited

Loading