Skip to content

Conversation

@4Ran8jith
Copy link
Contributor

integrates the DPNP framework into selected benchmarks

Copy link
Collaborator

@tbennun tbennun left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! Overall it looks good. However, there are some important notes on the readme that need to be addressed, as well as other comments that I left.

README.md Outdated
## Quickstart

To install NPBench, simply execute:
IF Dpnp is critical to you, see below. Otherwise, to install NPBench, simply execute:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dpnp is one framework out of several, please refrain from starting the quickstart section with that.

README.md Outdated
## Quickstart

To install NPBench, simply execute:
IF Dpnp is critical to you, see below. Otherwise, to install NPBench, simply execute:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dpnp is one framework out of several, please refrain from starting the quickstart section with that.

README.md Outdated
Comment on lines 168 to 176
```bibtex
@inproceedings{
npbench-dpnp,
author = {Ranjith et al.},
year = {2021},
title = {In prep.}
}
```

Copy link
Collaborator

Choose a reason for hiding this comment

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

The only publication that needs to be cited if using NPBench is NPBench; if, e.g., Numba is used, then Numba will be cited separately.

main.py Outdated
@@ -0,0 +1,72 @@
import argparse
Copy link
Collaborator

Choose a reason for hiding this comment

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

Renaming quickstart.py to main.py should be done as a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would just remove this file from the PR for now.

environment.yml Outdated
- https://software.repos.intel.com/python/conda
dependencies:
- python=3.10.14
- numpy=1.26.4 # dpnp theoretically requires numpy<=1.24.4, numba-dpex numpy>=1.26.4; seems that 1.26.4 works fine even dor dpnp
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- numpy=1.26.4 # dpnp theoretically requires numpy<=1.24.4, numba-dpex numpy>=1.26.4; seems that 1.26.4 works fine even dor dpnp
- numpy=1.26.4 # dpnp theoretically requires numpy<=1.24.4, numba-dpex numpy>=1.26.4; seems that 1.26.4 works fine even for dpnp

B[1:-1] = 0.33333 * (A[:-2] + A[1:-1] + A[2:])
A[1:-1] = 0.33333 * (B[:-2] + B[1:-1] + B[2:])

return A,B
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was a return value added to this benchmark?

def kernel(alpha, beta, A, B, C, D):

D[:] = alpha * A @ B @ C + beta * D
return D
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was a return value added to this benchmark?

README.md Outdated
With `dpnp` it is strongly recommended to use `conda` instead of `pip` for its dependency on intel packages.
Refer to this
[LINK](https://intelpython.github.io/dpnp/quick_start_guide.html#building-for-custom-sycl-targets) to know more
about building custom SYCL targets or installing `dpnp` package from intel channel.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
about building custom SYCL targets or installing `dpnp` package from intel channel.
about building custom SYCL targets or installing `dpnp` package from the `intel` channel.

README.md Outdated
Comment on lines 78 to 81
If you are behind proxies, follow your system documentation. E.g. for pip you may have to specify:
``` bash
$ python -m pip --proxy=http://localhost:1234 install <...> # where "localhost:1234" is the value of the env var "HTTP_PROXY"
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is unnecessary to include in the instructions. It is a pip issue rather than an NPBench issue.

@4Ran8jith
Copy link
Contributor Author

README.md: Updated as requested to include clarification about main.py and its purpose.

main.py: This file is designed specifically for benchmarks supporting DPNP only. It is intentionally not renamed to quickstart. A line has been added to README.md explaining this distinction.

environment.yml: Updated to reflect the latest changes.

Benchmarks: Existing benchmarks have been updated based on suggestions, and a new benchmark covariance2 has been added.

Return Statements: Modified some functions to properly use return to ensure correct copying where applicable.

Let me know if it's alright!

@4Ran8jith 4Ran8jith requested a review from tbennun April 1, 2025 10:01
Copy link
Contributor

@alexnick83 alexnick83 left a comment

Choose a reason for hiding this comment

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

Looks OK to me. I suggest that you remove this main.py file; we can amend quickstart separately at a later stage.

main.py Outdated
@@ -0,0 +1,72 @@
import argparse
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just remove this file from the PR for now.

@alexnick83 alexnick83 self-requested a review April 4, 2025 06:57
@alexnick83 alexnick83 merged commit 13e95c6 into spcl:main Apr 4, 2025
1 check passed
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.

5 participants