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

[oneDPL][RNG][Spec] Added the extensions sections #492

Merged

Conversation

paveldyakov
Copy link
Contributor

The "extensions section" with description of main differences between C++ std. RNG and oneDPL std. RNG has been added to oneAPI specification

@paveldyakov
Copy link
Contributor Author

@akukanov, @rarutyun, @aelizaro, could you please review the PR?

@akukanov akukanov self-requested a review August 25, 2023 14:48
Copy link
Contributor

@rarutyun rarutyun left a comment

Choose a reason for hiding this comment

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

The comments with colon are mostly suggestions, not strong preferences. I am not sure what is a right way, it just looks nicer to me.

Comment on lines +88 to +90
alternative requirements apply: for an engine object ``g`` of type ``G``,

- ``G::scalar_type`` is an unsigned integral type same as ``sycl::vec<Type,N>::element_type``,
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
alternative requirements apply: for an engine object ``g`` of type ``G``,
- ``G::scalar_type`` is an unsigned integral type same as ``sycl::vec<Type,N>::element_type``,
alternative requirements apply.
For an engine object ``g`` of type ``G``:
- ``G::scalar_type`` is an unsigned integral type same as ``sycl::vec<Type,N>::element_type``,

Copy link
Contributor

Choose a reason for hiding this comment

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

I am afraid, in this way it's not quite clear what the alternative requirements are. Let's try improving it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Postpone the change

source/elements/oneDPL/source/sycl_kernels_api.rst Outdated Show resolved Hide resolved
source/elements/oneDPL/source/sycl_kernels_api.rst Outdated Show resolved Hide resolved
- ``operator()`` of a distribution returns a ``sycl::vec<Type,N>`` filled with random values
in the closed interval ``[D::min(), D::max()]``;

The following engines and engine adaptors with predefined parameters are defined:
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
The following engines and engine adaptors with predefined parameters are defined:
The following template aliases for engines and engine adaptors are defined:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The wording "with predefined parameters" is aligned with C++ std. Should we change it?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I think we should keep it. From the RNG standpoint, it's exactly engines with predefined parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kept it as it is now

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree, because in the standard they don't have template aliases to the best of my knowledge. But I don't want to argue.

Copy link
Contributor

@akukanov akukanov Sep 11, 2023

Choose a reason for hiding this comment

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

https://eel.is/c++draft/rand.predef

using minstd_rand0 =
      linear_congruential_engine<uint_fast32_t, 16'807, 0, 2'147'483'647>;

and so on. OK, not template, just aliases.

source/elements/oneDPL/source/sycl_kernels_api.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@timmiesmith timmiesmith left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@rscohn2 rscohn2 merged commit cf9bc8f into uxlfoundation:main Sep 11, 2023
2 checks passed
aepanchi pushed a commit to aepanchi/oneAPI-spec that referenced this pull request Oct 31, 2023
* [oneDPL][RNG][Spec] Added the extensions sections

* Applying the comments

* Applying changes from Alexey

* Fixing the lines lengthexey

* Fix of the line

* Applying the comments from Ruslan
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