Skip to content
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

Implementing PUT routine #582

Merged
merged 40 commits into from
Oct 19, 2022
Merged

Implementing PUT routine #582

merged 40 commits into from
Oct 19, 2022

Conversation

ipdemes
Copy link
Contributor

@ipdemes ipdemes commented Sep 12, 2022

Implementing PUT method
The implementation reuses WRAP task (with some modifications) to compute points to the source array that need to be updated with the Values.
Since Legate Copy operation modifies values asynchronously, we can't guarantee that the output array will be the same as output from numpy.put for the case when there are duplicates in indices. Therefore, I have added the check for duplicate indices and warning message if they are found.

@ipdemes ipdemes changed the title Implementing PUT routine WIP: Implementing PUT routine Sep 12, 2022
@ipdemes ipdemes added the category:new-feature PR introduces a new feature and will be classified as such in release notes label Sep 12, 2022
@ipdemes ipdemes changed the title WIP: Implementing PUT routine Implementing PUT routine Sep 14, 2022
cunumeric/array.py Outdated Show resolved Hide resolved
cunumeric/array.py Outdated Show resolved Hide resolved
cunumeric/array.py Outdated Show resolved Hide resolved
Copy link
Contributor

@bryevdv bryevdv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo the bad copy/merge that needs fixed in array.py

cunumeric/deferred.py Outdated Show resolved Hide resolved
cunumeric/deferred.py Outdated Show resolved Hide resolved
cunumeric/module.py Outdated Show resolved Hide resolved
cunumeric/module.py Outdated Show resolved Hide resolved
@ipdemes
Copy link
Contributor Author

ipdemes commented Oct 7, 2022

@manopapad : thank you for finding the error. I am working on the fix

@manopapad : the fix for this use case is blocked on #582 (comment) issue

@ipdemes
Copy link
Contributor Author

ipdemes commented Oct 7, 2022

@manopapad I think I have addressed all the comments/issues with my latest commits.

rhs = src.base
else:
lhs = self.base
rhs = src._broadcast(lhs.shape)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added this logic for the case when lhs has shape () and rhs has shape (1,). Otherwise we get an error

Copy link
Contributor

@manopapad manopapad Oct 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still required? I just ran the test suite without this change and everything seems to be working.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is tested. I will add a test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I can't really test it on high level. I will revert the change since we are not using it

cunumeric/array.py Outdated Show resolved Hide resolved
@manopapad
Copy link
Contributor

This is ready to be merged, except for one remaining question above. I am noting here some work we should address in future PRs:

(1) We can skip the out-of-bounds checks inside of the task if mode != "raise".

(2) There are currently two different values in the "wrap" files that are both confusingly referred to as "input"/"in". I would like us to rename all values referring to the target array as "tgt" or "base", to make the code easier to understand. E.g. input_rect in wrap_template.inl would become base_rect, pitches_in in wrap.cc would become pitches_base etc.

(3) Raising an exception within an OpenMP block causes that exception to be caught and reported at the end of that block, instead of propagating it to our catch statement https://github.com/nv-legate/legate.core/blob/branch-22.12/src/core/task/task.h#L111. This causes code like the following to report an exception inside C++ instead of propagating that exception to python:

x = cn.zeros((10,))
cn.put(x, [6,7,8,9,10], 1)

Note that this likely affects zip as well.

@ipdemes
Copy link
Contributor Author

ipdemes commented Oct 18, 2022

@magnatelee : thank you, I have created an issue for the remaining improvements/fixes

@ipdemes ipdemes merged commit 87c7d45 into nv-legate:branch-22.12 Oct 19, 2022
@ipdemes ipdemes deleted the put branch January 12, 2023 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-feature PR introduces a new feature and will be classified as such in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants