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

Improve ScatterView Documentation #212

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ajpowelsnl
Copy link
Contributor

@ajpowelsnl ajpowelsnl commented Nov 28, 2022

This PR aims to improve readability by spelling out concepts represented by terse, non-standard acronyms (e.g., RT, RP) and linking to View documentation. Additionally, a brief usage explanation is provided, as are deprecation TODO around Kokkos::Impl statements.

@ajpowelsnl ajpowelsnl changed the title ScatterView.md: improve readability, link to View, add TODO Improve ScatterView Documentation Nov 28, 2022
friend class ScatterAccess<DataType, Op, ExecSpace, Layout, {ScatterNonDuplicated,ScatterDuplicated}, contribution, ScatterAtomic>;
friend class ScatterAccess<DataType, Operation, ExecSpace, Layout, {ScatterNonDuplicated,ScatterDuplicated}, contribution, ScatterNonAtomic>;
friend class ScatterAccess<DataType, Operation, ExecSpace, Layout, {ScatterNonDuplicated,ScatterDuplicated}, contribution, ScatterAtomic>;
[]:# (TODO: Deprecate types requiring `Kokkos::Impl..` )
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to commit this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the comment line, []:# (TODO: Deprecate types requiring Kokkos::Impl.. )? If yes, that was my initial intent, but I can remove it.

Copy link
Member

Choose a reason for hiding this comment

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

No I mean "commit". Are you sure about the syntax? The line did show on the rendered diff


template <typename... P, typename... Dims>
ScatterView(::Kokkos::Impl::ViewCtorProp<P...> const& arg_prop, Dims... dims);
[]: # (TODO: Deprecate types requiring `Kokkos::Impl..` )
Copy link
Member

Choose a reason for hiding this comment

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

What is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another comment line. I will remove it.

@@ -23,53 +28,53 @@ Kokkos::Experimental::contribute(results, scatter);
```c++

template <typename DataType
,int Op
,int Operation
,typename ExecSpace
,typename Layout
,int contribution
>
class ScatterView<DataType
Copy link
Member

@crtrott crtrott Dec 12, 2022

Choose a reason for hiding this comment

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

remove all the template arguments blower here i.e. lets just make this:

template<class DataType, class Layout, class ExecSpace, class Operation, class Duplication, class Contribution>
class ScatterView {

And then use Contribution instead of {ScatterNonDuplicated, ScatterDuplicated} that just didn't make any sense to write it the way it was.

Then you need to introduce a Parameters section like in Kokkos::View, before describing the typedefs.
In the parameters section you describe what the parameters are.

Copy link
Contributor Author

@ajpowelsnl ajpowelsnl Sep 5, 2024

Choose a reason for hiding this comment

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

@crtrott - apologies for the delayed update. I'm not sure what the class Duplication parameter specifies -- would potential values for this parameter be ScatterNonDuplicated, ScatterDuplicated? In any case, I've cleaned the entry up, and tried to get it into a basically correct and current state.

.. rubric:: Public Member Variables
* ``Layout``: See `Kokkos View LayoutType <../core/view/view.html>`_

* ``ExecutionSpace``: Where the code will be executed (CPU or GPU); typical values are ``Kokkos::DefaultHostExecutionSpace``, ``Kokkos::DefaultExecutionSpace``
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of listing typical values, I would just mention that this defaults to Kokkos::DefaultExecutionSpace if not specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@@ -96,18 +91,36 @@ Description
resize a view with discarding old data


.. rubric:: *Private* Members
Constructors
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this constructor subsection should be placed before the public class methods subsection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

6 participants