Broadcast array arg in binary ops if it's a valid leaf array type #51

kaushikcfd · 2021-06-28T01:24:36Z

Closes #49.

alexfikl · 2021-06-28T01:45:07Z

Is the "not device scalar" case useful?

In the general case, DOFArray will have different groups with different numbers of elements and degrees of freedom, so doing DOFArray + my_cl_array is unlikely to work, right?

kaushikcfd · 2021-06-28T01:51:18Z

In the general case, DOFArray will have different groups with different numbers of elements and degrees of freedom, so doing DOFArray + my_cl_array is unlikely to work, right?

If the broadcasting for any of the leaf arrays is illegal, we would see an error, which IMO is a reasonable user experience.

arraycontext/container/arithmetic.py

arraycontext/context.py

inducer

Thanks @alexfikl for taking a look. A few more thoughts below.

inducer · 2021-06-28T04:14:03Z

arraycontext/container/arithmetic.py

@@ -343,6 +343,17 @@ def {fname}(arg1):
                                else:
                                    raise ValueError(msg)""")
                    gen(f"return cls({zip_init_args})")
+                if _cls_has_array_context_attr:


This should get its own flag, defaulted to the same value as bcast_number.

Done, see here: 93013c9.

inducer · 2021-06-28T04:14:58Z

arraycontext/container/arithmetic.py

@@ -343,6 +343,17 @@ def {fname}(arg1):
                                else:
                                    raise ValueError(msg)""")
                    gen(f"return cls({zip_init_args})")
+                if _cls_has_array_context_attr:
+                    gen("if isinstance(arg2,"


Ordering here matters a great deal. (Maybe there should be a comment stating this.) These cases should be sorted from most likely to least. Is this the second-most-likely case?

IMO, this use-case would come just after host-scalars, as implemented in 93013c9.

inducer · 2021-06-28T04:16:13Z

arraycontext/container/arithmetic.py

+                                        lambda a: {op_str.format("a", "arg2")},
+                                        arg1))
+                            """)
+
                gen(f"""
                if {bool(outer_bcast_type_names)}:  # optimized away
                    if isinstance(arg2, {tup_str(outer_bcast_type_names)}):


This should also work (and be tested) for the reverse operators.

Added a test in f0e0587.

arraycontext/container/arithmetic.py

alexfikl

Looks good to me!

Just left a very small nitpick.

arraycontext/container/arithmetic.py

Co-authored-by: Alex Fikl <alexfikl@gmail.com>

replaced with only if Co-authored-by: Alex Fikl <alexfikl@gmail.com>

inducer

Thanks! Some comments below.

inducer · 2021-07-05T18:50:59Z

arraycontext/context.py

+    def get_array_types(self):
+        """
+        Returns a :class:`tuple` of types that are valid base array classes
+        the context can operate on.
+        """
+        return ()


I think this could be an attribute, to avoid the function call overhead. (If you need non-global import to set it, set it in the constructor.)

I think documenting this is probably OK.

Would it be sensible to only allow one type here, to avoid the splat above?

Done. 9255f22

Would it be sensible to only allow one type here, to avoid the splat above?

I think there is some value in keeping it tuple, default value is much nicer since the only intent is to perform type checking using it + also accounts for the slightest of chances that an array context might not have a single base array type.

test/test_arraycontext.py

arraycontext/container/arithmetic.py

inducer · 2021-07-05T19:05:10Z

arraycontext/container/arithmetic.py

+                if bcast_actx_array_type:
+                    all_outer_bcast_type_names = (
+                        outer_bcast_type_names
+                        + ("*arg1.array_context.get_array_types()",))
+                else:
+                    all_outer_bcast_type_names = outer_bcast_type_names


Move this closer to the usage site.

Make this produce a tuple that's either empty or has the actx array type(s) in it. Add both together in the argument of tup_str.

(Same below for reverse.)

Thanks, I think 2040a8d makes it better.

inducer · 2021-07-05T19:05:43Z

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

Co-authored-by: Andreas Kloeckner <andreask@illinois.edu>

inducer

Thanks! Just a few more minor things, then this is ready to go.

arraycontext/context.py

arraycontext/container/arithmetic.py

inducer · 2021-07-06T00:03:20Z

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

This reverts commit e6b8b1b.

kaushikcfd marked this pull request as ready for review June 28, 2021 01:35

kaushikcfd requested review from alexfikl and inducer June 28, 2021 01:35

alexfikl reviewed Jun 28, 2021

View reviewed changes

arraycontext/container/arithmetic.py Outdated Show resolved Hide resolved

arraycontext/context.py Outdated Show resolved Hide resolved

inducer reviewed Jun 28, 2021

View reviewed changes

inducer mentioned this pull request Jun 28, 2021

Broadcasting rules distinguish between host scalar and device scalars #49

Closed

kaushikcfd force-pushed the fix_49 branch from 6523dfd to f0e0587 Compare June 29, 2021 01:35

kaushikcfd requested review from alexfikl and inducer June 29, 2021 01:39

alexfikl approved these changes Jun 29, 2021

View reviewed changes

arraycontext/container/arithmetic.py Outdated Show resolved Hide resolved

kaushikcfd force-pushed the fix_49 branch from ccda800 to e045c34 Compare June 30, 2021 19:13

kaushikcfd and others added 2 commits June 30, 2021 16:16

broadcast arithmetic if arg2 is actx's array

fec4b5b

define and implement get_array_types

174b62d

Co-authored-by: Alex Fikl <alexfikl@gmail.com>

kaushikcfd force-pushed the fix_49 branch from e045c34 to 8adf707 Compare June 30, 2021 21:18

kaushikcfd and others added 2 commits June 30, 2021 17:10

adds test_leaf_array_type_broadcasting

4492d5f

avoid using iff in the docs

4b93ef3

replaced with only if Co-authored-by: Alex Fikl <alexfikl@gmail.com>

kaushikcfd force-pushed the fix_49 branch from 8adf707 to 4b93ef3 Compare June 30, 2021 22:10

kaushikcfd mentioned this pull request Jul 2, 2021

Makes array operations ArrayContext oblivious illinois-ceesd/drivers_y1-nozzle#7

Closed

2 tasks

Merge branch 'main' into fix_49

ef5edd4

inducer reviewed Jul 5, 2021

View reviewed changes

kaushikcfd and others added 5 commits July 5, 2021 15:28

adds docs for test, tests with scalars

974c242

ArrayContext.get_array_types() -> ArrayContext.array_types

9255f22

bcast_actx_array_type -> _bcast_actx_array_type

96392c0

Co-authored-by: Andreas Kloeckner <andreask@illinois.edu>

formatting: remove unused import

658fadb

leaf array bcast types: better code placement

2040a8d

kaushikcfd force-pushed the fix_49 branch from 13ad2bd to 2040a8d Compare July 5, 2021 21:04

kaushikcfd requested a review from inducer July 5, 2021 21:06

inducer reviewed Jul 6, 2021

View reviewed changes

arraycontext/context.py Outdated Show resolved Hide resolved

arraycontext/context.py Outdated Show resolved Hide resolved

arraycontext/container/arithmetic.py Outdated Show resolved Hide resolved

arraycontext/container/arithmetic.py Outdated Show resolved Hide resolved

kaushikcfd added 3 commits July 5, 2021 19:53

make array_types a property

e6b8b1b

docs: grammar

eabe193

better var naming: ary_types -> bcast_actx_ary_types

9eae9a9

kaushikcfd requested a review from inducer July 6, 2021 00:56

inducer added 2 commits July 6, 2021 00:16

Revert "make array_types a property"

c647360

This reverts commit e6b8b1b.

ArrayContext: make array_types a class attribute

8702db6

inducer merged commit be0f569 into main Jul 6, 2021

inducer deleted the fix_49 branch July 6, 2021 14:40

alexfikl mentioned this pull request Jul 7, 2021

Remove hacks for device scalars inducer/pytential#104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Broadcast array arg in binary ops if it's a valid leaf array type #51

Broadcast array arg in binary ops if it's a valid leaf array type #51

kaushikcfd commented Jun 28, 2021 •

edited by inducer

Loading

alexfikl commented Jun 28, 2021

kaushikcfd commented Jun 28, 2021

inducer left a comment

inducer Jun 28, 2021

kaushikcfd Jun 29, 2021

inducer Jun 28, 2021

kaushikcfd Jun 29, 2021

inducer Jun 28, 2021

kaushikcfd Jun 29, 2021

alexfikl left a comment

inducer left a comment

inducer Jul 5, 2021

kaushikcfd Jul 5, 2021 •

edited

Loading

inducer Jul 5, 2021

kaushikcfd Jul 5, 2021 •

edited

Loading

inducer commented Jul 5, 2021

inducer left a comment

inducer commented Jul 6, 2021

Broadcast array arg in binary ops if it's a valid leaf array type #51

Broadcast array arg in binary ops if it's a valid leaf array type #51

Conversation

kaushikcfd commented Jun 28, 2021 • edited by inducer Loading

alexfikl commented Jun 28, 2021

kaushikcfd commented Jun 28, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexfikl left a 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

kaushikcfd Jul 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaushikcfd Jul 5, 2021 • edited Loading

Choose a reason for hiding this comment

inducer commented Jul 5, 2021

inducer left a comment

Choose a reason for hiding this comment

inducer commented Jul 6, 2021

kaushikcfd commented Jun 28, 2021 •

edited by inducer

Loading

kaushikcfd Jul 5, 2021 •

edited

Loading

kaushikcfd Jul 5, 2021 •

edited

Loading