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

DOC: Array API support #1918

Merged
merged 15 commits into from
Oct 21, 2024
Merged

Conversation

samir-nasibli
Copy link
Contributor

@samir-nasibli samir-nasibli commented Jul 5, 2024

Description

Docs update for functional support of array API in 2025.0 rls version

  • I have reviewed my changes thoroughly before submitting this pull request.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes, if necessary (updated in # - add PR number)
  • The unit tests pass successfully.
  • I have run it locally and tested the changes extensively.
  • I have resolved any merge conflicts that might occur with the base branch.
  • Git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details)
  • I have added a respective label(s) to PR if I have a permission for that.

Copy link
Contributor

@icfaust icfaust left a comment

Choose a reason for hiding this comment

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

Here is a deep review of the text. We can still aim to have this integrated today.

doc/sources/array_api.rst Outdated Show resolved Hide resolved
doc/sources/array_api.rst Outdated Show resolved Hide resolved
doc/sources/array_api.rst Outdated Show resolved Hide resolved
doc/sources/array_api.rst Outdated Show resolved Hide resolved
doc/sources/array_api.rst Outdated Show resolved Hide resolved
doc/sources/array_api.rst Outdated Show resolved Hide resolved
doc/sources/array_api.rst Outdated Show resolved Hide resolved
doc/sources/array_api.rst Outdated Show resolved Hide resolved
doc/sources/array_api.rst Outdated Show resolved Hide resolved
doc/sources/array_api.rst Outdated Show resolved Hide resolved
@samir-nasibli
Copy link
Contributor Author

/intelci: run

@samir-nasibli
Copy link
Contributor Author

/intelci: run

For new example run validation.

doc/sources/array_api.rst Outdated Show resolved Hide resolved
tests/run_examples.py Show resolved Hide resolved
doc/sources/array_api.rst Outdated Show resolved Hide resolved
@samir-nasibli
Copy link
Contributor Author

/intelci: run

Copy link
Contributor

@icfaust icfaust left a comment

Choose a reason for hiding this comment

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

Further english suggestions.

doc/sources/array_api.rst Outdated Show resolved Hide resolved
doc/sources/array_api.rst Outdated Show resolved Hide resolved
doc/sources/array_api.rst Outdated Show resolved Hide resolved
doc/sources/array_api.rst Outdated Show resolved Hide resolved
doc/sources/array_api.rst Outdated Show resolved Hide resolved
`Data Parallel Control usm_ndarray <https://intelpython.github.io/dpctl/latest/index.html>`__ all computation
will be only accomplished on CPU.

Stock scikit-learn uses `config_context(array_api_dispatch=True)` for enabling Array API `support <https://scikit-learn.org/1.5/modules/array_api.html>`__.
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
Stock scikit-learn uses `config_context(array_api_dispatch=True)` for enabling Array API `support <https://scikit-learn.org/1.5/modules/array_api.html>`__.
Scikit-learn uses `config_context(array_api_dispatch=True)` for enabling Array API `support <https://scikit-learn.org/1.5/modules/array_api.html>`__.

Copy link
Contributor

Choose a reason for hiding this comment

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

Double check capitalization of Scikit-learn vs Scikit-Learn in this file, it could be that my suggestions to 'Scikit-Learn' are incorrect, but they need to be consistent.

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 would like to leave stock prefix here just for the context.
Everywhere in our docs and repository Scikit-learn is used. If updates are required then that alignment could be done as a follow up in 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.

What does stock mean in this case?

Copy link
Contributor Author

@samir-nasibli samir-nasibli requested a review from icfaust October 18, 2024 12:46
@samir-nasibli
Copy link
Contributor Author

/intelci: run

doc/sources/array_api.rst Outdated Show resolved Hide resolved
doc/sources/array_api.rst Outdated Show resolved Hide resolved
Comment on lines 110 to 111
`Data Parallel Control usm_ndarray <https://intelpython.github.io/dpctl/latest/index.html>`__ all computation
will be only accomplished on CPU unless specified by a config_context with an available GPU device.
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
`Data Parallel Control usm_ndarray <https://intelpython.github.io/dpctl/latest/index.html>`__ all computation
will be only accomplished on CPU unless specified by a config_context with an available GPU device.
`Data Parallel Control usm_ndarray <https://intelpython.github.io/dpctl/latest/index.html>`__ all computations
will be performed only on CPU unless specified by a config_context with an available GPU device.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why this suggestion is needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not strictly needed, but I'd say it sounds more grammatical.

reorder the sections
@samir-nasibli
Copy link
Contributor Author

/intelci: run

Copy link
Contributor

@icfaust icfaust left a comment

Choose a reason for hiding this comment

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

Dependent on Davids concerns being properly addressed.

@samir-nasibli
Copy link
Contributor Author

/intelci: run

doc/sources/array_api.rst Outdated Show resolved Hide resolved
doc/sources/array_api.rst Outdated Show resolved Hide resolved
@samir-nasibli samir-nasibli merged commit cde94da into uxlfoundation:main Oct 21, 2024
9 of 12 checks passed
@samir-nasibli samir-nasibli deleted the doc/array_api branch October 21, 2024 13:16
@samir-nasibli
Copy link
Contributor Author

@mergify backport rls/2025.0.0-rls

Copy link

mergify bot commented Oct 21, 2024

backport rls/2025.0.0-rls

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Oct 21, 2024
* DOC: Array API suport

* Example for DBSCAN array API

(cherry picked from commit cde94da)

# Conflicts:
#	doc/sources/index.rst
@mergify mergify bot mentioned this pull request Oct 21, 2024
8 tasks
samir-nasibli added a commit that referenced this pull request Oct 22, 2024
* DOC: Array API support (#1918)

* Example for DBSCAN array API

(cherry picked from commit cde94da)

# Conflicts:
#	doc/sources/index.rst

* fix merge conflict

---------

Co-authored-by: Samir Nasibli <samir.nasibli@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants