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

docs: add docstring #567

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

docs: add docstring #567

wants to merge 4 commits into from

Conversation

jokasimr
Copy link
Contributor

@jokasimr jokasimr commented Nov 7, 2024

No description provided.

Copy link
Member

@jl-wynen jl-wynen 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, but I take this opportunity to be pedantic about style, generally following PEP257 and pydocstyle / ruff

quadrature_kind:
What kind of quadrature to use.
A denser quadrature makes the result more accurate but takes longer to compute.
What options exists depend on the sample shape.
Copy link
Member

Choose a reason for hiding this comment

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

You can use a Literal as the type of quadrature_kind to tell the users which arguments are possible. As it stands, it is very difficult to find that out.

src/scippneutron/absorption/base.py Show resolved Hide resolved
Parameters
----------
sample_shape:
the size and shape of the sample
Copy link
Member

Choose a reason for hiding this comment

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

Capitalization and punctuation are inconsistent here. Please make every entry in Parameters (and the return documentation) one or more sentences. I.e., begin with a capital letter and end with a period.

@@ -15,6 +15,33 @@ def compute_transmission_map(
detector_position: sc.Variable,
quadrature_kind: Any = 'medium',
) -> sc.DataArray:
"""
Computes the fraction of neutrons transmitted through the sample
given that they scattered to the positions in the ``detector_positions`` array.
Copy link
Member

Choose a reason for hiding this comment

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

The first sentence of a docstring should only be a single line and it should be its own paragraph, i.e., separated by an empty line from the rest. (See also https://peps.python.org/pep-0257/#multi-line-docstrings) Sphinx rtelies on this and shows the first paragraph in summary tables. And there is little space there. So please split it up and write a concise summary plus an expanded explanation.

Typically, the summary uses imperative mood. But we don't do that consistently in Scipp.

@jokasimr
Copy link
Contributor Author

jokasimr commented Nov 7, 2024

Looks good, but I take this opportunity to be pedantic about style

Please be!

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.

2 participants