Implement broadcast* routines #458

sbak5 · 2022-07-19T00:02:32Z

Implementation of the following routines

broadcast_shapes
broadcast_to
broadcast_arrays
broadcast
These routines follow the behavior of the corresponding numpy routines.

…e routines

ipdemes · 2022-07-19T18:29:09Z

@sbak5 : it seems like all the tests a failing. Could you, please, have a look ?

sbak5 · 2022-07-19T18:30:25Z

@sbak5 : it seems like all the tests a failing. Could you, please, have a look ?

Yes. Already prepared a fix and testing on a local machine before pushing it.

cunumeric/array.py

ipdemes · 2022-07-20T16:25:00Z

@sbak5 Please add new api to documentation here and try to build documentation (cd docs/cunumeric; make clean html)

ipdemes · 2022-07-20T19:06:59Z

I think there are tests for broadcast_to, broadcast_shapes and broadcast_arrays missing. Could you, please, add them?

cunumeric/module.py

sbak5 · 2022-07-20T20:29:00Z

I think there are tests for broadcast_to, broadcast_shapes and broadcast_arrays missing. Could you, please, add them?

broadcast simply does broadcast_arrays, which uses broadcast_to.
But I can add tests for these to make sure the routines work well in separate tests.

ipdemes · 2022-07-20T20:37:14Z

But I can add tests for these to make sure the routines work well in separate tests.

Yes, I think it would be nice to have separate tests

cunumeric/module.py

ipdemes · 2022-07-21T01:44:54Z

Can you replace all calls to np.broadcast_shapes in exiting cuNumeric code yo use your new broadcast_shape method?

cunumeric/module.py

…es` w/ `_broadcast_shapes`

tests/integration/test_broadcast.py

cunumeric/module.py

behave correctly for an array manipulation routine

sbak5 · 2022-09-09T00:02:14Z

@bryevdv Can you review the changes again? If it looks fine, I'll merge it. @ipdemes has already reviewed the changes for both your and her comments.

cunumeric/module.py

tests/integration/test_broadcast.py

tests/integration/test_flags.py

cunumeric/_ufunc/ufunc.py

cunumeric/array.py

magnatelee · 2022-10-05T19:34:22Z

cunumeric/eager.py

        else:
            self.array = np.flip(rhs.array, axes)

+    def broadcast_to(self, shape: NdShape) -> NumPyThunk:


this code doesn't make sense at all. why is an eager array always converted to a deferred array here? can't this be implemented using numpy.broadcast_to?

As far as I remember, this was intended to handle a case when eager and deferred array are used for a operation. In this case, a broadcasted eager array should be changed to a corresponding deferred array, which doesn't work w/ deferred.broadcast_to. So, I changed this code to convert eager array to deferred when broadcast_tois called. I couldn't find a way to handle this case on my own w/o this change.

magnatelee · 2022-10-05T19:39:52Z

cunumeric/module.py

+    broadcasted: bool = False,
+) -> ndarray:
+    # create an array object w/ options passed from 'broadcast' routines
+    arr = array(arr, copy=False, subok=subok)


what's the point of this call? note that subok isn't handled in array anyway, so this is a no-op.

broadcast has the argument and numpy.array in cuNumeric still has the argument even though it's not implemented for subclasses. This call is just aligned with the corresponding routines so that there's no need to change this call when subclasses are implemented.

@magnatelee do you have a follow-up to the previous reply? I don't have enough context to know what is appropriate here (and below)

magnatelee · 2022-10-05T19:40:08Z

cunumeric/module.py

+    subok: bool = False,
+) -> list[ndarray]:
+    # create an arry object w/ options passed from 'broadcast' routines
+    arrays = [array(arr, copy=False, subok=subok) for arr in arrs]


same comment here. why is this necessary?

cunumeric/module.py

magnatelee · 2022-10-05T19:56:04Z

@sbak5 it looks like NumPy's writeable flag is more liberal than that in this PR. For example, you can make a view from a np.broadcast_to call writeable by calling setflags(write=True) on it. And there shouldn't be any problem for cuNumeric to allow it. But this PR requires that owndata be True to be able to toggle the writeable flag, whereas NumPy doesn't need that. So I think we should revisit whether that's really necessary and also think about whether there are any true read-only views that cannot be turned into writeable views in any case.

sbak5 · 2022-11-01T18:09:51Z

@sbak5 it looks like NumPy's writeable flag is more liberal than that in this PR. For example, you can make a view from a np.broadcast_to call writeable by calling setflags(write=True) on it. And there shouldn't be any problem for cuNumeric to allow it. But this PR requires that owndata be True to be able to toggle the writeable flag, whereas NumPy doesn't need that. So I think we should revisit whether that's really necessary and also think about whether there are any true read-only views that cannot be turned into writeable views in any case.

This is because of the runtime issue in legion as far as I remember.
When you create a view from broadcast_to and set write=True, there was issue in legion runtime regarding the ordering of tasks for write requests. @ipdemes found the issue and suggested me this change as far as I recall.
@ipdemes can you elaborate the issue here?

for more information, see https://pre-commit.ci

bryevdv · 2023-01-19T20:22:59Z

As a start, brought up to date with current branch-22.12.

Co-authored-by: Wonchan Lee <Magnate.Lee@gmail.com>

for more information, see https://pre-commit.ci

…into sbak5/broadcast

bryevdv · 2023-01-19T23:20:48Z

@ipdemes @magnatelee I have made a quick stab at bringing this PR up to date with the latest branch, and also addressing some of the low-hanging suggestions. There's a lot of comments and history here. For my own sake, would you mind if I copied all of this over to a new PR branch and cleaned up the commit history with an interactive rebase, then started on a new review there?

magnatelee · 2023-01-20T00:27:34Z

For my own sake, would you mind if I copied all of this over to a new PR branch and cleaned up the commit history with an interactive rebase, then started on a new review there?

Not at all. I'd be happy to do a fresh review. Please go open a new PR.

bryevdv · 2023-01-20T19:35:39Z

closing, superseded by #759

* enhance for random --------- Co-authored-by: Jacob Faibussowitsch <jacob.fai@gmail.com>

sbak5 added 3 commits July 18, 2022 16:51

[WIP] broadcast routines

d7fbd1f

Added a test for np.broadcast

774cc2f

Refined broadcast routines across modules and added description of th…

a99ab61

…e routines

sbak5 requested review from ipdemes and magnatelee July 19, 2022 00:02

sbak5 added 2 commits July 18, 2022 17:15

Fixed a typo in _broadcast_arrays

fecdde3

Changed 'DeferredArray.broadcast_to' to fix bugs

26b4ebb

sbak5 added 2 commits July 19, 2022 12:04

Include changes for bugs in DeferredArray

54bcd8f

Delete commented line

30d5537

ipdemes reviewed Jul 20, 2022

View reviewed changes

cunumeric/array.py Outdated Show resolved Hide resolved

ipdemes reviewed Jul 20, 2022

View reviewed changes

cunumeric/module.py Show resolved Hide resolved

ipdemes reviewed Jul 20, 2022

View reviewed changes

cunumeric/module.py Outdated Show resolved Hide resolved

ipdemes reviewed Jul 21, 2022

View reviewed changes

cunumeric/module.py Outdated Show resolved Hide resolved

ipdemes reviewed Jul 21, 2022

View reviewed changes

cunumeric/module.py Outdated Show resolved Hide resolved

ipdemes reviewed Jul 21, 2022

View reviewed changes

cunumeric/module.py Outdated Show resolved Hide resolved

sbak5 added 4 commits July 21, 2022 16:31

Added tests for broadcast* routines and replaced `np.broadcast_shap…

d7004ef

…es` w/ `_broadcast_shapes`

Merge branch 'branch-22.07' into sbak5/broadcast

ec783db

Delete the import of typing

ac064dc

Added the broadcast routines in the document

5b6917f

ipdemes reviewed Jul 22, 2022

View reviewed changes

tests/integration/test_broadcast.py Outdated Show resolved Hide resolved

ipdemes reviewed Jul 22, 2022

View reviewed changes

cunumeric/module.py Outdated Show resolved Hide resolved

sbak5 added 2 commits July 22, 2022 16:17

Made the input array shapes a bit more comprehensive

eb8760b

Merge branch 'branch-22.07' into sbak5/broadcast

6610d2f

Added a test case to check if the outcome of broadcasting

9987ece

behave correctly for an array manipulation routine

bryevdv reviewed Sep 9, 2022

View reviewed changes

cunumeric/module.py Show resolved Hide resolved

tests/integration/test_broadcast.py Outdated Show resolved Hide resolved

tests/integration/test_flags.py Show resolved Hide resolved

Collect all error cases in _check_result

9fdb719

bryevdv approved these changes Sep 13, 2022

View reviewed changes

manopapad mentioned this pull request Sep 18, 2022

cunumeric/module: implement numpy.meshgrid #587

Open

magnatelee requested changes Oct 5, 2022

View reviewed changes

bryevdv and others added 2 commits January 19, 2023 11:51

Merge branch 'branch-22.12' into sbak5/broadcast

752bc95

[pre-commit.ci] auto fixes from pre-commit.com hooks

077f97d

for more information, see https://pre-commit.ci

bryevdv changed the base branch from branch-22.10 to branch-22.12 January 19, 2023 20:21

bryevdv and others added 11 commits January 19, 2023 12:24

remove unrelated whitespace

aed9bff

remove superfluous helper function

5cb1ab8

use more dims in test

46e819c

move check_writeable to _prepare_operands

fc11335

Apply suggestions from code review

46d7706

Co-authored-by: Wonchan Lee <Magnate.Lee@gmail.com>

[pre-commit.ci] auto fixes from pre-commit.com hooks

2f24d22

for more information, see https://pre-commit.ci

Merge branch 'sbak5/broadcast' of https://github.com/sbak5/cunumeric …

dfd42e2

…into sbak5/broadcast

fix flagsobj and flags tests

379c90d

use ValueError for writeable error

811ea68

remove boilerplate from _diag_helper

d75bbbe

copy method and assertions for flagobj

1da66f1

bryevdv mentioned this pull request Jan 20, 2023

Implement broadcast routines #759

Merged

bryevdv closed this Jan 20, 2023

XiaLuNV added a commit to XiaLuNV/cunumeric that referenced this pull request Dec 13, 2024

enhance for random (nv-legate#458)

a8dd4a7

* enhance for random --------- Co-authored-by: Jacob Faibussowitsch <jacob.fai@gmail.com>

Implement broadcast* routines #458

Implement broadcast* routines #458

Uh oh!

Conversation

sbak5 commented Jul 19, 2022

Uh oh!

ipdemes commented Jul 19, 2022

Uh oh!

sbak5 commented Jul 19, 2022

Uh oh!

Uh oh!

ipdemes commented Jul 20, 2022

Uh oh!

ipdemes commented Jul 20, 2022

Uh oh!

Uh oh!

sbak5 commented Jul 20, 2022

Uh oh!

ipdemes commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ipdemes commented Jul 21, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbak5 commented Sep 9, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

magnatelee Oct 5, 2022

Choose a reason for hiding this comment

Uh oh!

sbak5 Nov 1, 2022

Choose a reason for hiding this comment

Uh oh!

magnatelee Oct 5, 2022

Choose a reason for hiding this comment

Uh oh!

sbak5 Nov 1, 2022

Choose a reason for hiding this comment

Uh oh!

bryevdv Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

magnatelee Oct 5, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

magnatelee commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbak5 commented Nov 1, 2022

Uh oh!

bryevdv commented Jan 19, 2023

Uh oh!

bryevdv commented Jan 19, 2023

Uh oh!

magnatelee commented Jan 20, 2023

Uh oh!

bryevdv commented Jan 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

ipdemes commented Jul 20, 2022 •

edited

Loading

magnatelee commented Oct 5, 2022 •

edited

Loading