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

Add best practices info to sphinx docs #1048

Merged
merged 3 commits into from
Sep 22, 2023

Conversation

bryevdv
Copy link
Contributor

@bryevdv bryevdv commented Sep 20, 2023

This PR adds the "Best Practices" doc authored by @shriram-jagan to the deployed docs User Guide.

@bryevdv bryevdv added the enhancement New feature or request label Sep 20, 2023
@bryevdv bryevdv added category:improvement PR introduces an improvement and will be classified as such in release notes and removed enhancement New feature or request labels Sep 20, 2023
@bryevdv
Copy link
Contributor Author

bryevdv commented Sep 20, 2023

I'd recommend building docs locally and inspecting the results directly, but here are a few representative screenshots

image


image

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 typos and such, otherwise LGTM

docs/cunumeric/source/user/practices.rst Outdated Show resolved Hide resolved
docs/cunumeric/source/user/practices.rst Outdated Show resolved Hide resolved
General Recommendations
-----------------------

Following the basics of numpy as documented here is highly recommended. Here,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Following the basics of numpy as documented here is highly recommended. Here,
Following the basics of numpy as documented [here](https://numpy.org/doc/stable/user/basics.html) is highly recommended. Here,

docs/cunumeric/source/user/practices.rst Outdated Show resolved Hide resolved
docs/cunumeric/source/user/practices.rst Outdated Show resolved Hide resolved
docs/cunumeric/source/user/practices.rst Outdated Show resolved Hide resolved
Co-authored-by: Manolis Papadakis <[email protected]>
Copy link
Contributor

@shriram-jagan shriram-jagan left a comment

Choose a reason for hiding this comment

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

Mostly nits. Looks good to me



In the example below, the function ``transform`` is defined to operate on
calars. But it can also be used on an array to linearly transform its elements,
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: this should be scalars and not calars


When the array needs to be updated from another array based on a condition
that they both satisfy, use ``putmask`` for better performance. Unlike the
previous example, here x is set to twice the value of y when the condition
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should we say x instead of x and similarly for y?

x[np.logical_and(first_cond, second_cond)] = const


Refer to the documentation for other logical operations.
Copy link
Contributor

Choose a reason for hiding this comment

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

docs/cunumeric/source/user/practices.rst Show resolved Hide resolved
@manopapad manopapad merged commit 0b11958 into nv-legate:branch-23.09 Sep 22, 2023
23 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:improvement PR introduces an improvement and will be classified as such in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants