UniqueToken.md: API documentation for UniqueToken.md #179

ajpowelsnl · 2022-10-03T22:47:41Z

This PR is the first draft of API documentation for UniqueToken capability and is linked to the #5140 "catch all" Documentation issue.

JBludau

there is a lot of whitespace in the c++ code that looks akward given the style we use in the rest of the documentation.

docs/source/API/core/parallel-dispatch/UniqueToken.md

crtrott · 2022-11-28T18:47:51Z

Document the extra constructor for UniqueTokenScope==Global as:

UniqueToken(size_type max_size, execution_space space = execution_space()) requires(TokenScope == Global);

ajpowelsnl · 2024-09-05T17:49:56Z

Document the extra constructor for UniqueTokenScope==Global as:
UniqueToken(size_type max_size, execution_space space = execution_space()) requires(TokenScope == Global);

Done

JBludau

Looking at the implementation I want to suggest some changes. But someone should verify.

docs/source/API/core/parallel-dispatch/UniqueToken.rst

JBludau · 2024-09-07T10:28:01Z

docs/source/API/core/parallel-dispatch/UniqueToken.rst

+    int id = token . acquire ();
+    RandomGen gen = pool (id );
+    // Computation Body
+    token . release (id );


Suggested change

int id = token . acquire ();

RandomGen gen = pool (id );

// Computation Body

token . release (id );

auto id = token.acquire ();

RandomGen gen = pool (id );

// Computation Body

token.release (id );

docs/source/API/core/parallel-dispatch/UniqueToken.rst

JBludau · 2024-09-07T10:48:54Z

Document the extra constructor for UniqueTokenScope==Global as:
UniqueToken(size_type max_size, execution_space space = execution_space()) requires(TokenScope == Global);

Shouldn't it be TokenScope == Instance for the ctor to take a max_size?

ajpowelsnl · 2024-11-11T16:42:33Z

@JBludau - I used most of your suggestions. See what you think about the updates.

JBludau

looks good to me

dalg24 · 2024-11-11T19:06:47Z

docs/source/API/core/parallel-dispatch/UniqueToken.rst

+   In a parallel region, before the main computation, a pool of ``UniqueToken`` (integer) ID is generated, and each ID is released following iteration.
+
+.. warning::
+   ``UniqueToken <ExecutionSpace> token`` *can* be called inside a parallel region, *but* must be released at the end of *each* iteration.


Should we be saying "work item" instead of "iteration"?

Either formulation is fine : " ... each iterate" or " ... each work item"

dalg24 · 2024-11-11T19:52:56Z

docs/source/API/core/parallel-dispatch/UniqueToken.rst

+*  ``UniqueTokenScope``:  defaults to ``Instance``, and every instance is independent of another.  In contrast, ``Global`` uses one set of unique IDs for all instances.
+
+.. note::
+   In a parallel region, before the main computation, a pool of ``UniqueToken`` (integer) ID is generated, and each ID is released following iteration.


each ID is released following iteration.

I don't quite understand what this means.

I guess she meant to say that the token is released at the end of the iteration

I reworked for clarity.

dalg24 · 2024-11-11T19:56:14Z

docs/source/API/core/parallel-dispatch/UniqueToken.rst

+     UniqueToken(size_type max_size, ExecutionSpace execution_space = ExecutionSpace())
+     // Scope is instance
+
+  .. code-block:: cpp
+
+     UniqueToken(size_type max_size, ExecutionSpace execution_space = ExecutionSpace()) requires(TokenScope == Global); 
+     // Scope is global


I am not following what is the difference between the two.
Should that be interpreted as, regardless of whether TokenScope is instance or global, the constructor takes an integer argument and optionally an execution space trailing argument that defaults to the default exec space instance?

I think are right, the constructor that takes a max_size will instantiate UniqueToken<ExecutionSpace,UniqueTokenScope::Instance>.
The one with only the ExecutionSpace as argument does not restrict the scope.

yes -- the main distinction is scope (instance or global).

dalg24 · 2024-11-11T19:56:36Z

docs/source/API/core/parallel-dispatch/UniqueToken.rst

+ .. code-block:: cpp
+
+    UniqueToken(size_type max_size, ExecutionSpace execution_space=ExecutionSpace())
+


Suggested change

.. code-block:: cpp

UniqueToken(size_type max_size, ExecutionSpace execution_space=ExecutionSpace())

presumably a copy pasta error

dalg24 · 2024-11-11T19:57:23Z

docs/source/API/core/parallel-dispatch/UniqueToken.rst

+
+ .. code-block:: cpp
+
+    size_type size()


Did we forget to define size_type?

dalg24 · 2024-11-11T19:59:18Z

docs/source/API/core/parallel-dispatch/UniqueToken.rst

+ .. code-block:: cpp
+
+    size_type size()
+    // Returns the size of the token pool    


There is a dedicated syntax for return

Suggested change

// Returns the size of the token pool

:returns: the size of the token pool

:return: doesn't work (as a Sphinx directive) with .. code-block:: cpp or .. cpp:function::

https://www.sphinx-doc.org/en/master/usage/domains/cpp.html#info-field-lists

Here, code-block:: cpp (vs. cpp:function::) is being used (i.e., cpp:function requires a conventional function signature (vs. method on a class).

JBludau

good catch by @dalg24, the ctor description is not correct

JBludau · 2024-11-11T20:26:45Z

docs/source/API/core/parallel-dispatch/UniqueToken.rst

+  .. code-block:: cpp
+
+     UniqueToken(size_type max_size, ExecutionSpace execution_space = ExecutionSpace())
+     // Scope is instance
+
+  .. code-block:: cpp
+
+     UniqueToken(size_type max_size, ExecutionSpace execution_space = ExecutionSpace()) requires(TokenScope == Global); 
+     // Scope is global


Suggested change

.. code-block:: cpp

UniqueToken(size_type max_size, ExecutionSpace execution_space = ExecutionSpace())

// Scope is instance

.. code-block:: cpp

UniqueToken(size_type max_size, ExecutionSpace execution_space = ExecutionSpace()) requires(TokenScope == Global);

// Scope is global

.. code-block:: cpp

UniqueToken(size_type max_size, ExecutionSpace execution_space = ExecutionSpace{})

// Scope is instance

UniqueToken(ExecutionSpace execution_space = ExecutionSpace{})

// Scope can be instance or global

JBludau · 2024-11-12T14:06:06Z

docs/source/API/core/parallel-dispatch/UniqueToken.rst

+*  ``UniqueTokenScope``:  defaults to ``Instance``, and every instance is independent of another.  In contrast, ``Global`` uses one set of unique IDs for all instances.
+
+.. note::
+   In a parallel region, before the main computation, a pool of ``UniqueToken`` (integer) ID is generated.  A generated ID is released following iteration (see ``void release(size_t idx)`` below).


I am not sure if we need the note when we have the warning below

I would prefer if we kept the warning and ditched the note. The warning was very explicit about what users have to adhere to. The note makes it sound optional in my opinion

ajpowelsnl requested review from fnrizzi and nliber October 3, 2022 22:47

JBludau requested changes Oct 17, 2022

View reviewed changes

ajpowelsnl requested a review from JBludau September 5, 2024 17:35

ajpowelsnl requested review from crtrott and cedricchevalier19 September 5, 2024 17:50

JBludau reviewed Sep 7, 2024

View reviewed changes

ajpowelsnl added 3 commits November 7, 2024 14:09

UniqueToken.md: API documentation for UniqueToken.md

d63ea5b

UniqueToken.rst: first round of editing & making simpatico w/ Sphinx

6910dff

UniqueToken.rst: add class constructor

d5f1d97

ajpowelsnl force-pushed the documentation/UniqueToken branch from 713ac08 to d5f1d97 Compare November 11, 2024 16:40

UniqueToken.rst: incorporating Jakob B's suggestion

cc05ccc

JBludau approved these changes Nov 11, 2024

View reviewed changes

dalg24 reviewed Nov 11, 2024

View reviewed changes

JBludau requested changes Nov 11, 2024

View reviewed changes

ajpowelsnl added 2 commits November 11, 2024 15:58

UniqueToken.rst: address DLG comments

25a2ef3

UniqueToken.rst: fix constructors

0156d10

JBludau reviewed Nov 12, 2024

View reviewed changes

UniqueToken.rst: rm warning to reduce redundancy

39292ec

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UniqueToken.md: API documentation for UniqueToken.md #179

UniqueToken.md: API documentation for UniqueToken.md #179

ajpowelsnl commented Oct 3, 2022

JBludau left a comment

crtrott commented Nov 28, 2022

ajpowelsnl commented Sep 5, 2024

JBludau left a comment

JBludau Sep 7, 2024

JBludau commented Sep 7, 2024

ajpowelsnl commented Nov 11, 2024

JBludau left a comment

dalg24 Nov 11, 2024

ajpowelsnl Nov 11, 2024

dalg24 Nov 11, 2024

JBludau Nov 11, 2024

ajpowelsnl Nov 11, 2024

dalg24 Nov 11, 2024

JBludau Nov 11, 2024

ajpowelsnl Nov 11, 2024

dalg24 Nov 11, 2024

dalg24 Nov 11, 2024

dalg24 Nov 11, 2024

ajpowelsnl Nov 11, 2024

dalg24 Nov 12, 2024

ajpowelsnl Nov 12, 2024 •

edited

Loading

JBludau left a comment

JBludau Nov 11, 2024

JBludau Nov 12, 2024

ajpowelsnl Nov 12, 2024

JBludau Nov 12, 2024

		.. code-block:: cpp

		UniqueToken(size_type max_size, ExecutionSpace execution_space=ExecutionSpace())

	// Returns the size of the token pool
	:returns: the size of the token pool

UniqueToken.md: API documentation for UniqueToken.md #179

Are you sure you want to change the base?

UniqueToken.md: API documentation for UniqueToken.md #179

Conversation

ajpowelsnl commented Oct 3, 2022

JBludau left a comment

Choose a reason for hiding this comment

crtrott commented Nov 28, 2022

ajpowelsnl commented Sep 5, 2024

JBludau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JBludau commented Sep 7, 2024

ajpowelsnl commented Nov 11, 2024

JBludau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajpowelsnl Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

JBludau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajpowelsnl Nov 12, 2024 •

edited

Loading