-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Array container support and batched (eager) communication #154
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments to help explain my thoughts.
The Mirgecom CI failure seems to be due to using the Edit: I don't know if it's something fundamentally wrong with |
Looks like map_array_container is not catching the I'll try to come up with a fix this afternoon. In the meantime, you can probably work around it by checking 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? |
28480c4
to
9127937
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! A few wrinkles (hopefully easy to answer/fix) in the communication bit, but otherwise this looks nice!
Unsubscribing... @-mention or request review once it's ready for a look or needs attention. |
d2cbd52
to
91eec65
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
grudge/op.py
Outdated
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think _div_helper
deals with anything other than object arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
grudge/op.py
Outdated
: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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think _div_helper
deals with anything other than object arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Unsubscribing... @-mention or request review once it's ready for a look or needs attention. |
Co-authored-by: Andreas Klöckner <inform@tiker.net>
451979c
to
8893ea6
Compare
8893ea6
to
c1e5148
Compare
@inducer This is ready for another look |
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
Thanks for seeing this through! In it goes. 🎉 |
This PR modifies existing grudge routines to work on arbitrary array containers and batches eager MPI communication of containers.
Requires: inducer/arraycontext#91