Return device scalars from `nodal_sum`/`nodal_min`/`nodal_max` #170

majosm · 2021-09-24T20:22:32Z

Closes #168.

⚠️ Merge together with illinois-ceesd/mirgecom#518. ⚠️

thomasgibson

Thanks for doing this @majosm --- I saw no major issues after scanning through this PR. How should we proceed forward with respect to illinois-ceesd/mirgecom#518? Probably merge this first, followed immediately with the mirgecom PR? If so, let's coordinate so we can time this right.

.github/workflows/ci.yml

majosm · 2021-10-20T15:09:08Z

Thanks for doing this @majosm --- I saw no major issues after scanning through this PR. How should we proceed forward with respect to illinois-ceesd/mirgecom#518? Probably merge this first, followed immediately with the mirgecom PR? If so, let's coordinate so we can time this right.

Yeah, I think we would merge this, then remove the requirements.txt change in illinois-ceesd/mirgecom#518, run its CI once more, and then merge it. Afterwards we can revert the ci.yml change from this PR. Do I have this right @inducer?

alexfikl

Left a couple of comments for some things I'm not quite sure about, but overall 👍!

I haven't been keeping up with grudge and maybe some of them aren't relevant, so feel free to ignore as needed!

grudge/dt_utils.py

grudge/reductions.py

thomasgibson

Sorry @majosm but I sort of had a change of heart with regard to type annotations using Any

thomasgibson · 2021-10-20T18:35:06Z

grudge/dt_utils.py

@@ -158,7 +159,7 @@ def dt_non_geometric_factors(

 @memoize_on_first_arg
 def h_max_from_volume(
-        dcoll: DiscretizationCollection, dim=None, dd=None) -> float:
+        dcoll: DiscretizationCollection, dim=None, dd=None) -> Any:


Actually I feel like type annotating with Any is not very helpful. In fact, I feel it just sorta raises more questions. I'd rather just remove the return annotation (at least for h_max and h_min here).

Maybe try a DeviceScalar = Any type alias?

https://www.python.org/dev/peps/pep-0484/#type-aliases

Mypy shouldn't be bothered, but I don't know if it'll render nicely in Sphinx.

You should be able to trick sphinx into it too using autodoc_type_aliases.

That seems to require from __future__ import annotations, which is a >=3.7 thing, so probably needs to be hidden behind an if BUILDING_DOCS or something?

from __future__ import annotations should be unnecessary if you make the annotation a string. (I think)

What about using the ArrayT type hint from arraycontext? Something like: majosm@0e1be13?

From the point of view of the type checker, they're probably interchangable here. Using a single unbound TypeVar in a function definition doesn't actually constrain anything, you need something like

def function(x: T) -> Sequence[T]: ...

i.e. it should tie in more than one input + output, for it to do something. (at least that's my understanding of it)

Since this is mostly for documentation at this point, either one is fine from my point of view (at least can't think of any particular downside?)

Added the DeviceScalar thing directly in arraycontext in inducer/arraycontext#117, since other code may want to use it too.

Without from __future__ import annotations, sphinx seems to be quite limited about where it allows type aliases, but pylint and mypy work as expected.

inducer/arraycontext#117 is in.

Am I doing this right? (e8fd358)

👍 It seems to be rendering just fine on my machine too.

grudge/reductions.py

Stale review

not needed anymore after pyopencl update

… is only referenced once again

inducer

LGTM!

inducer · 2021-10-25T18:43:16Z

grudge/dt_utils.py

@@ -158,7 +159,7 @@ def dt_non_geometric_factors(

 @memoize_on_first_arg
 def h_max_from_volume(
-        dcoll: DiscretizationCollection, dim=None, dd=None) -> float:
+        dcoll: DiscretizationCollection, dim=None, dd=None) -> Any:


Maybe try a DeviceScalar = Any type alias?

https://www.python.org/dev/peps/pep-0484/#type-aliases

Mypy shouldn't be bothered, but I don't know if it'll render nicely in Sphinx.

grudge/models/__init__.py

majosm · 2021-10-28T17:22:47Z

(Looks like I don't have merge permissions for grudge.)

inducer · 2021-10-29T02:31:58Z

(Looks like I don't have merge permissions for grudge.)

Added you.

majosm force-pushed the nodal-reduction-device-scalar branch from fb65e9c to 8b00650 Compare September 24, 2021 20:24

majosm mentioned this pull request Oct 11, 2021

Account for grudge nodal reductions returning device scalars illinois-ceesd/mirgecom#518

Merged

8 tasks

majosm force-pushed the nodal-reduction-device-scalar branch 2 times, most recently from 2609903 to 7a1b391 Compare October 13, 2021 19:31

thomasgibson mentioned this pull request Oct 15, 2021

Overintegration support for operators and reductions #172

Closed

1 task

majosm force-pushed the nodal-reduction-device-scalar branch 2 times, most recently from 6902796 to e5f8d9d Compare October 19, 2021 14:32

majosm added 6 commits October 19, 2021 09:33

return device scalar from nodal_sum/nodal_min/nodal_max

c0efd84

handle force_device_scalars==False case

d7386da

fix type annotations

eb65fa2

temporarily change downstream mirgecom CI branch

0158b0c

add comment explaining why abs() is outside of the sum

9886993

use actx.np.sqrt instead of ^(1/2)

ea69dec

majosm force-pushed the nodal-reduction-device-scalar branch from e5f8d9d to ea69dec Compare October 19, 2021 14:33

thomasgibson mentioned this pull request Oct 20, 2021

Array container support and batched (eager) communication #154

Merged

majosm marked this pull request as ready for review October 20, 2021 14:33

thomasgibson previously approved these changes Oct 20, 2021

View reviewed changes

thomasgibson reviewed Oct 20, 2021

View reviewed changes

.github/workflows/ci.yml Show resolved Hide resolved

alexfikl reviewed Oct 20, 2021

View reviewed changes

grudge/dt_utils.py Outdated Show resolved Hide resolved

grudge/reductions.py Show resolved Hide resolved

grudge/reductions.py Outdated Show resolved Hide resolved

grudge/reductions.py Outdated Show resolved Hide resolved

return device scalar from h_min/max_from_volume

2e6b66c

thomasgibson self-requested a review October 20, 2021 15:53

majosm added 2 commits October 20, 2021 11:37

add back return type hints

dbcec03

attempt to fix force_device_scalars handling

214e865

thomasgibson reviewed Oct 20, 2021

View reviewed changes

majosm force-pushed the nodal-reduction-device-scalar branch from 1778902 to 214e865 Compare October 20, 2021 20:57

majosm added 3 commits October 21, 2021 09:33

remove force_device_scalars handling

a3f54cb

not needed anymore after pyopencl update

restore old code in h_min_from_volume/h_max_from_volume now that actx…

cd82971

… is only referenced once again

restore missing return type hint

0725cec

inducer approved these changes Oct 25, 2021

View reviewed changes

add a few missing numpy conversions

b106569

majosm force-pushed the nodal-reduction-device-scalar branch from ed86d15 to a9477fe Compare October 25, 2021 21:18

return device scalar from estimate_rk4_timestep

a2c54f0

majosm force-pushed the nodal-reduction-device-scalar branch from a9477fe to a2c54f0 Compare October 25, 2021 22:39

use arraycontext.DeviceScalar type annotation

e8fd358

majosm requested a review from inducer October 28, 2021 17:21

thomasgibson merged commit a788ad6 into inducer:main Oct 28, 2021

MTCam mentioned this pull request Oct 29, 2021

CI is failing illinois-ceesd/mirgecom#539

Closed

This was referenced Oct 29, 2021

Mirgecom downstream CI didn't detect breakage #180

Closed

Production CI uses requirements.txt from main illinois-ceesd/mirgecom#541

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return device scalars from `nodal_sum`/`nodal_min`/`nodal_max` #170

Return device scalars from `nodal_sum`/`nodal_min`/`nodal_max` #170

majosm commented Sep 24, 2021 •

edited

Loading

thomasgibson left a comment

majosm commented Oct 20, 2021

alexfikl left a comment

thomasgibson left a comment

thomasgibson Oct 20, 2021

inducer Oct 25, 2021

alexfikl Oct 25, 2021

inducer Oct 25, 2021 •

edited

Loading

majosm Oct 25, 2021

alexfikl Oct 27, 2021

alexfikl Oct 27, 2021

inducer Oct 27, 2021

majosm Oct 28, 2021

alexfikl Oct 28, 2021

inducer left a comment

inducer Oct 25, 2021

majosm commented Oct 28, 2021

inducer commented Oct 29, 2021

Return device scalars from nodal_sum/nodal_min/nodal_max #170

Return device scalars from nodal_sum/nodal_min/nodal_max #170

Conversation

majosm commented Sep 24, 2021 • edited Loading

thomasgibson left a comment

Choose a reason for hiding this comment

majosm commented Oct 20, 2021

alexfikl left a comment

Choose a reason for hiding this comment

thomasgibson 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

inducer Oct 25, 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

inducer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majosm commented Oct 28, 2021

inducer commented Oct 29, 2021

Return device scalars from `nodal_sum`/`nodal_min`/`nodal_max` #170

Return device scalars from `nodal_sum`/`nodal_min`/`nodal_max` #170

majosm commented Sep 24, 2021 •

edited

Loading

inducer Oct 25, 2021 •

edited

Loading