-
Notifications
You must be signed in to change notification settings - Fork 424
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 stencilDist documentation #26472
Conversation
The new wording is heavily based on an example provided by Damian. Thanks! Signed-off-by: Ben Harshbarger <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, see my comments. Thanks Damian!
It is interesting that the writeup focuses on the "why" whereas I wanted to go straight to the "what" (or the "how"?). This is me personally, the "why" may work better for StencilDist users.
modules/dists/StencilDist.chpl
Outdated
For example, solving banded linear equations by the Gauss-Seidel or Jacobi | ||
method. | ||
|
||
Like ``blockDist``, each of the blocks in a ``stencilDist`` array are owned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"are" --> "is"
I would even shorten "each of the blocks" to "each block".
modules/dists/StencilDist.chpl
Outdated
The indices are partitioned in the same way as the :mod:`blockDist | ||
The ``stencilDist`` distribution is an enhanced functionality variant of the | ||
:mod:`blockDist<BlockDist>` distribution. Both refer to an array partitioned | ||
into blocks, but ``stencilDist`` will internally attempt to improve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Internally" sounds too close to "automatically". The latter is not the case, right? How about "helps user to" instead?
modules/dists/StencilDist.chpl
Outdated
between a given block and its set of immediately neighboring or adjacent | ||
blocks, the more feature-full ``stencilDist`` most likely makes it the better | ||
choice. This is because, for any given block, a ``stencilDist`` transparently | ||
creates a read-only cache of selected array elements from its adjacent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, maybe add "user-managed" about the cache, or something similar?
I see it says below that the user must updateFluff()
, however what if the reader misses this later part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's enough other documentation about this to leave the line as-is, especially in the "Updating the Cached Elements" section later on. This sentence is also pretty close to being too long already.
modules/dists/StencilDist.chpl
Outdated
strictly, as 'fluff'. This approach can avoid many fine-grained GETs and | ||
PUTs when performing a stencil computation near the boundary of the current | ||
locale's chunk of array elements. The user must manually refresh these caches | ||
after writes by calling the ``updateFluff`` method. Otherwise, reading and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Otherwise" here sounds a bit like "if the user has not refreshed the caches." I suggest instead "Reading and writing array elements owned by the current locale".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with "Writing ..." instead, since we just finished describing how reading is different 😄 . I also merged the sentence about "indices" into this one.
modules/dists/StencilDist.chpl
Outdated
just outside the edge of the locally-owned block. This documentation may | ||
refer to this cached band of array elements as either 'ghost cells' or more | ||
strictly, as 'fluff'. This approach can avoid many fine-grained GETs and | ||
PUTs when performing a stencil computation near the boundary of the current |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"gets and puts" are slang to the user. How about "fine-grained communication" or "reads and writes of remote array elements" ?
Signed-off-by: Ben Harshbarger <[email protected]>
Thanks for the feedback! I've pushed some changes that hopefully address the points above. I also included an explicit line about the term "halo" since Damian found that valuable (and was part of his original suggestion). |
Signed-off-by: Ben Harshbarger <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go. Thank you Ben for implementing Damian's suggestions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a few minor wording suggestions, but agree that this is an improvement, and don't consider either of my changes strictly necessary.
Signed-off-by: Ben Harshbarger <[email protected]>
The new wording is heavily based on an example provided by Damian. Thanks!