Array container support and batched (eager) communication by thomasgibson · Pull Request #154 · inducer/grudge

thomasgibson · 2021-08-17T14:09:19Z

This PR modifies existing grudge routines to work on arbitrary array containers and batches eager MPI communication of containers.

~~Requires: inducer/arraycontext#91~~

alexfikl

Gave this a very quick look and left a few comments here and there, hope you don't mind!

The TracePair stuff seems a bit more complicated, so I mostly skipped it 😁

thomasgibson

Some comments to help explain my thoughts.

thomasgibson · 2021-09-21T15:44:25Z

The Mirgecom CI failure seems to be due to using the map_array_container in the operator application routines (not the distributed memory bits). Will investigate this...

Edit: I don't know if it's something fundamentally wrong with map_array_container, but for some reason, the map_array_container is attempting to serialize floats!?

alexfikl · 2021-09-21T16:08:13Z

The Mirgecom CI failure seems to be due to using the map_array_container in the operator application routines (not the distributed memory bits). Will investigate this...

Edit: I don't know if it's something fundamentally wrong with map_array_container, but for some reason, the map_array_container is attempting to serialize floats!?

Looks like map_array_container is not catching the NotImplementedError for floats correctly and trying to deserialize them anyway. I'd say this is a bug in arraycontext.

I'll try to come up with a fix this afternoon. In the meantime, you can probably work around it by checking not isinstance(vec, DOFArray) and is_array_container(vec) before calling map_array_container (?).

EDIT: Actually, that check needs to be expanded anyway, otherwise it looks a lot like an infinite loop if you give it a float to start with?

thomasgibson · 2021-09-21T16:52:56Z

@alexfikl I think it was a very subtle type-checking issue! I managed to fix the mrigecom issue with this commit: 42adaf1

inducer

Thanks for working on this! A few wrinkles (hopefully easy to answer/fix) in the communication bit, but otherwise this looks nice!

inducer · 2021-09-23T05:04:09Z

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

thomasgibson · 2021-11-03T01:19:41Z

@inducer when this lands, do not delete this branch; I will take care of it since I need to reset the base branch in #172

inducer

Thanks, and sorry again about the long wait on another review. Also sorry that I've uncovered a few more things. I hope most of this is easy. There are a lot of comments here, but most of them are just about consistency in doing the same thing in multiple places. I hope you'll be able to dispatch them quickly using the "apply suggestions" feature.

inducer · 2021-11-03T23:13:43Z

-        a :class:`~meshmode.dof_array.DOFArray`\ s,
+    :arg vecs: an object array of
+        :class:`~meshmode.dof_array.DOFArray`\s or
+        :class:`~arraycontext.container.ArrayContainer` objects,


I don't think _div_helper deals with anything other than object arrays.

I believe this is all cleared up in a877340. _div_helper is intentionally only working on object arrays, as the container is expected to have vector (obj array) entries.

inducer · 2021-11-03T23:14:10Z

+    :arg vecs: an object array of
+        :class:`~meshmode.dof_array.DOFArray`\ s or
+        :class:`~arraycontext.container.ArrayContainer` objects,
        where the last axis of the array must have length
        matching the volume dimension.
-    :returns: a :class:`~meshmode.dof_array.DOFArray`.
+    :returns: a :class:`~meshmode.dof_array.DOFArray` or an
+        :class:`~arraycontext.container.ArrayContainer` of them.


I don't think _div_helper deals with anything other than object arrays.

I believe this is all cleared up in a877340. _div_helper is intentionally only working on object arrays, as the container is expected to have vector (obj array) entries.

inducer · 2021-11-03T23:49:55Z

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

Co-authored-by: Andreas Klöckner <inform@tiker.net>

thomasgibson · 2021-11-12T00:23:35Z

@inducer This is ready for another look

inducer · 2021-12-01T20:33:05Z

Thanks for working on this! Some proposed changes to this in #191. This LGTM (and is ready to land in my view) subject to those (or similar) changes.

* Refactor grad/weak grad to use the same helper, simplify div empty handling * Factor out some redundant code in elementwise reductions

inducer · 2021-12-06T22:57:09Z

Thanks for seeing this through! In it goes. 🎉

thomasgibson linked an issue Aug 17, 2021 that may be closed by this pull request

Teach grudge about array containers #97

Closed

thomasgibson mentioned this pull request Aug 17, 2021

Eager distributed: support array containers #136

Closed

This was linked to issues Sep 4, 2021

Eager distributed: support array containers #136

Closed

Eager distributed: Send array containers together #137

Closed

thomasgibson commented Sep 4, 2021

View reviewed changes

Comment thread grudge/trace_pair.py Outdated

Comment thread grudge/trace_pair.py Outdated

alexfikl reviewed Sep 8, 2021

View reviewed changes

thomasgibson requested a review from inducer September 16, 2021 19:08

thomasgibson marked this pull request as ready for review September 16, 2021 19:09

thomasgibson commented Sep 20, 2021

View reviewed changes

Comment thread grudge/trace_pair.py Outdated

Comment thread grudge/trace_pair.py Outdated

thomasgibson self-assigned this Sep 20, 2021

alexfikl mentioned this pull request Sep 21, 2021

Fix map_array_container on non-ArrayContainers inducer/arraycontext#90

Merged

thomasgibson changed the title ~~Array container support~~ Array container support and batched (eager) communication Sep 21, 2021

thomasgibson force-pushed the thg/array-containers branch from 28480c4 to 9127937 Compare September 23, 2021 00:46

inducer reviewed Sep 23, 2021

View reviewed changes

Comment thread grudge/trace_pair.py Outdated

Comment thread grudge/trace_pair.py Outdated

Comment thread grudge/trace_pair.py Outdated

Comment thread grudge/trace_pair.py Outdated

thomasgibson requested a review from inducer September 23, 2021 16:34

alexfikl reviewed Sep 23, 2021

View reviewed changes

Comment thread grudge/trace_pair.py Outdated

This was referenced Sep 24, 2021

Entropy-stable DG: flux differencing and SBP operators #169

Closed

Flatten entire array containers inducer/arraycontext#91

Merged

thomasgibson force-pushed the thg/array-containers branch from d2cbd52 to 91eec65 Compare September 27, 2021 02:04

thomasgibson added distributed Relating to distributed-memory support enhancement New feature or request labels Sep 27, 2021

thomasgibson added 5 commits October 19, 2021 11:51

Support array containers in core grudge DG ops

3b7e3da

Convert wave example to use array containers

f5123d9

Update projection routine to support array containers

aec5dc7

Convert mpi communication

5d77c5f

Use DOFArray-based traversal routines

2d083b6

thomasgibson requested a review from inducer October 28, 2021 18:15

thomasgibson added 4 commits October 28, 2021 15:51

Merge branch 'main' into thg/array-containers

91ba57b

Merge branch 'main' into thg/array-containers

a63b2e3

Merge branch 'main' into thg/array-containers

ffe7d1c

Merge branch 'main' into thg/array-containers

307a421

inducer reviewed Nov 3, 2021

View reviewed changes

thomasgibson and others added 4 commits November 3, 2021 21:51

Documentation updates

d3c51c5

Co-authored-by: Andreas Klöckner <inform@tiker.net>

Fix comment formatting

90bcf4a

Containerize grad/div functions

eeb80eb

Add checks for empty arrays in grad/div operators

cc32439

thomasgibson force-pushed the thg/array-containers branch from 451979c to 8893ea6 Compare November 11, 2021 19:56

Fix gradient op for vector-valued components

c1e5148

thomasgibson force-pushed the thg/array-containers branch from 8893ea6 to c1e5148 Compare November 11, 2021 20:02

thomasgibson added 2 commits November 11, 2021 17:16

Correct div helper for handling empty and nested obj arrays

a877340

Clear up documentation and add clarifying notes

e7478d4

thomasgibson requested a review from inducer November 12, 2021 00:24

inducer and others added 4 commits November 25, 2021 21:53

Merge branch 'main' into thg/array-containers

4619910

Merge branch 'main' into thg/array-containers

9460bff

Merge branch 'main' into thg/array-containers

237fc8a

Merge branch 'main' into thg/array-containers

3784d18

inducer mentioned this pull request Dec 1, 2021

More shared code in array container support #191

Merged

More shared code in array container support (#191)

dd06257

* Refactor grad/weak grad to use the same helper, simplify div empty handling * Factor out some redundant code in elementwise reductions

inducer enabled auto-merge (squash) December 6, 2021 22:56

inducer merged commit d48c9bc into main Dec 6, 2021

inducer deleted the thg/array-containers branch December 6, 2021 23:02

Conversation

thomasgibson commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexfikl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomasgibson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thomasgibson commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexfikl commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomasgibson commented Sep 21, 2021

Uh oh!

inducer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

inducer commented Sep 23, 2021

Uh oh!

Uh oh!

thomasgibson commented Nov 3, 2021

Uh oh!

inducer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

inducer Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

thomasgibson Nov 11, 2021

Choose a reason for hiding this comment

Uh oh!

inducer Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

thomasgibson Nov 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

inducer commented Nov 3, 2021

Uh oh!

thomasgibson commented Nov 12, 2021

Uh oh!

inducer commented Dec 1, 2021

Uh oh!

inducer commented Dec 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

thomasgibson commented Aug 17, 2021 •

edited

Loading

thomasgibson commented Sep 21, 2021 •

edited

Loading

alexfikl commented Sep 21, 2021 •

edited

Loading