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

Changing @stencil docs to correctly reflect func_or_mode param #3594

Merged
merged 4 commits into from Dec 18, 2018

Conversation

AaronCritchley
Copy link
Contributor

The existing comment looks inaccurate, the documentation below goes through the options for the @stencil decorator and there are more than the one option the doc would suggest. 😄

Let me know if I've misunderstood / misread though!

@stuartarchibald
Copy link
Contributor

Thanks for the PR. I think that the intention of the proposed deleted paragraph was to note that there is at present only one option for border handling mode="constant", the docs could be improved to make this more clear?

@AaronCritchley
Copy link
Contributor Author

Ah, that makes sense, I see that that's also covered in the mode docs too.

I've just noticed #3486 hits this section of the docs, so I'll change to use func_or_mode, would you prefer for me to reintroduce the removed paragraph as-is or make any adjustments?

@stuartarchibald
Copy link
Contributor

I've just noticed #3486 hits this section of the docs, so I'll change to use func_or_mode

yes please, I think that should be fine, the parallel version just wraps and manipulates the serial version so I think the declared kwargs are the same.

would you prefer for me to reintroduce the removed paragraph as-is or make any adjustments?

If you've any ideas on a way to make that clearer such that it reads in a manner that doesn't make it seem to refer to all the options (which I guess is the motivation for this PR?) then that would be great!

Many thanks.

@codecov-io
Copy link

Codecov Report

Merging #3594 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3594   +/-   ##
=======================================
  Coverage   80.68%   80.68%           
=======================================
  Files         393      393           
  Lines       80485    80485           
  Branches     9164     9164           
=======================================
  Hits        64942    64942           
  Misses      14127    14127           
  Partials     1416     1416

@AaronCritchley
Copy link
Contributor Author

I've re-added the original paragraph (my bad!) and modified the docs to point to func_or_mode - let me know if there's anything else I've missed!

@AaronCritchley AaronCritchley changed the title Removing inaccurate sentence in @stencil docs Changing @stencil docs to correctly reflect func_or_mode param Dec 13, 2018
--------

The optional mode parameter controls how the border of the output array
The optional func_or_mode parameter controls how the border of the output array
Copy link
Contributor

Choose a reason for hiding this comment

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

func_or_mode should probably have double back ticks around as it looks strange when rendered as it's clearly a parameter to a function (evident from underscores in name).

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! (Sorry for the delay)

@stuartarchibald
Copy link
Contributor

I've reviewed the text that was proposed as text for removal. How about doing the following...

Switch the text block to a note, and adjust the wording as follows:

.. note::
   The stencil decorator may be augmented in the future to provide additional
   mechanisms for border handling. At present, only one behaviour is
   implemented, ``"constant"`` (see ``func_or_mode`` below for details).

What do you think? Does this make the intended clearer?

@AaronCritchley
Copy link
Contributor Author

I've reviewed the text that was proposed as text for removal. How about doing the following...

Switch the text block to a note, and adjust the wording as follows:

.. note::
   The stencil decorator may be augmented in the future to provide additional
   mechanisms for border handling. At present, only one behaviour is
   implemented, ``"constant"`` (see ``func_or_mode`` below for details).

What do you think? Does this make the intended clearer?

Yep, looks great and lots clearer, thanks!

@@ -171,10 +172,10 @@ specified neighborhood, **the behavior is undefined.**

.. _stencil-mode:

``mode``
``func_or_mode``
Copy link
Contributor

Choose a reason for hiding this comment

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

Title underline needs extending to the same number of chars as the title. Docs are built with warning-as-error so I think this will fail to build.

Copy link
Contributor

Choose a reason for hiding this comment

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

@AaronCritchley, I've fixed this in b7d223f.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for fixing, sorry for not catching this, I didn't build the docs after my change, lesson learnt 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem. Thanks for instigating these changes, the stencil options read better now!

As title.
@seibert seibert merged commit cd0c974 into numba:master Dec 18, 2018
@AaronCritchley AaronCritchley deleted the stencil-doc-fix branch December 18, 2018 11:23
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.

None yet

4 participants