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

adding mask_indices routine #426

Merged
merged 13 commits into from
Jul 28, 2022
Merged

Conversation

ipdemes
Copy link
Contributor

@ipdemes ipdemes commented Jun 24, 2022

@magnatelee : is this implementation align with your suggestion?

@magnatelee
Copy link
Contributor

please add type annotations to your functions. (see #387 and #413)

cunumeric/module.py Outdated Show resolved Hide resolved
cunumeric/module.py Outdated Show resolved Hide resolved
cunumeric/module.py Outdated Show resolved Hide resolved
@magnatelee
Copy link
Contributor

@ipdemes please please don't force-push changes while the changes are under a review. doing so makes it extremely difficult to look back the history.

@ipdemes ipdemes requested a review from magnatelee July 4, 2022 02:30
cunumeric/module.py Outdated Show resolved Hide resolved
cunumeric/module.py Outdated Show resolved Hide resolved
cunumeric/module.py Outdated Show resolved Hide resolved
Copy link
Contributor

@manopapad manopapad left a comment

Choose a reason for hiding this comment

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

Some small documentation fixes, and a complaint about the warning, otherwise looks good.

Notes
-----
WARNING: `mask_indices` expects `mask_function` to call cuNumeric functions
for good performance. In case non-cuNumeric fucntions are called by
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: "functions"

@ipdemes ipdemes requested a review from manopapad July 27, 2022 18:21
Assume `mask_func` is a function that, for a square array a of size
``(n, n)`` with a possible offset argument `k`, when called as
``mask_func(a, k)`` returns a new array with zeros in certain locations
(functions like `triu` or `tril` do precisely this). Then this function
Copy link
Contributor

Choose a reason for hiding this comment

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

functions like :func:`cunumeric.triu` or :func:`cunumeric.tril`

Copy link
Contributor

@manopapad manopapad left a comment

Choose a reason for hiding this comment

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

See 2 unresolved comments for some remaining minor documentation fixes. Feel free to merge after that.

Edit: Actually, let's way for mypy to run in CI

@ipdemes
Copy link
Contributor Author

ipdemes commented Jul 28, 2022

@manopapad : all the tests pass after your commit. Can I merge this PR?

@manopapad
Copy link
Contributor

yes, please go ahead

@ipdemes ipdemes merged commit c11a44b into nv-legate:branch-22.07 Jul 28, 2022
jjwilke pushed a commit to jjwilke/cunumeric that referenced this pull request Jul 29, 2022
jjwilke pushed a commit to jjwilke/cunumeric that referenced this pull request Jul 29, 2022
sbak5 pushed a commit to sbak5/cunumeric that referenced this pull request Aug 17, 2022
@ipdemes ipdemes deleted the mask_indices 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants