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

Add small examples of common docstring styles #38117

Merged
merged 5 commits into from
Jun 9, 2024

Conversation

kwankyu
Copy link
Collaborator

@kwankyu kwankyu commented May 31, 2024

which I think are already established. But it is good to recommend preferred styles to keep docstrings in consistent styles.

motivated by the comment: #37821 (comment)

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

#38068 (merged here)

@kwankyu kwankyu changed the title Small amendments of coding style Small amendments of docstring style May 31, 2024
@kwankyu kwankyu changed the title Small amendments of docstring style Add small representative examples of common docstring style May 31, 2024
@kwankyu kwankyu changed the title Add small representative examples of common docstring style Add small examples of common docstring styles May 31, 2024
Copy link

github-actions bot commented May 31, 2024

Documentation preview for this PR (built with commit f4cb7cb; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

src/doc/en/developer/coding_basics.rst Outdated Show resolved Hide resolved
src/doc/en/developer/coding_basics.rst Outdated Show resolved Hide resolved
src/doc/en/developer/coding_basics.rst Outdated Show resolved Hide resolved
src/doc/en/developer/coding_basics.rst Outdated Show resolved Hide resolved
src/doc/en/developer/coding_basics.rst Show resolved Hide resolved
src/doc/en/developer/coding_basics.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@gmou3 gmou3 left a comment

Choose a reason for hiding this comment

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

Thanks. Looks good.

@kwankyu
Copy link
Collaborator Author

kwankyu commented May 31, 2024

Thanks. A good example of collaborative work :-)

@gmou3
Copy link
Contributor

gmou3 commented May 31, 2024

Let's give this a bit more time.

@saraedum we can continue the discussion here.

@gmou3 gmou3 requested a review from saraedum May 31, 2024 22:48
@kwankyu
Copy link
Collaborator Author

kwankyu commented Jun 1, 2024

Let's give this a bit more time.

@saraedum we can continue the discussion here.

OK. We can. If we reach to a consensus that we can implement, then we can make a new PR.

I am unsure about what the rule is/was.

I am too. There is no rule in sage community to decide what is a rule :-)

I think the part after the "--" should be treated the same as the text body preceding the "INPUT". One can see how to type the default value from the function header.

I agree with this.

My main argument is that the code base uses more double-backticks than single-backticks.

Of course, since most default values are None, True, or False. These are non-mathematical values.

I believe that ... should not set new standards but report what the code base is already doing.

I agree. The problem is that there are examples for each of different styles.

... 1 is the actual default value as a Python int, it's not really a "mathematical object" arguably. I believe that the defaults should report the actual default of a function, imho this can never be a mathematical object.

I would not say that any of

(A)

(default: 1) 

(B)

(default: ``1``)

(C)

(default: `1`)

does not follow a rule. It all depends on the author's intention about the python function he is writing. If the value has no mathematical meaning, (A) is okay. If the value is a python object, (B) is the only choice. If the value is mathematical, then (C) is nice.

If the function calculates an arc length of an ellipse and takes input a and b which are radians, the default values of a and b are mathematical, then clearly (C) is preferable.

In this example,

       - ``p`` -- prime integer (default: `2`); coprime with `n`

we want to say (C) is recommended.

@saraedum
Copy link
Member

saraedum commented Jun 1, 2024

My main argument is that the code base uses more double-backticks than single-backticks.

Of course, since most default values are None, True, or False. These are non-mathematical values.

No, that's not what I meant. Sorry, I should have been more specific. I grepped for numerical values only here. Even with just integers, I get:

git grep -E 'default: `[0-9]+`'|wc -l
75
git grep -E 'default: ``[0-9]+``'|wc -l
325

@saraedum
Copy link
Member

saraedum commented Jun 1, 2024

I am unsure about what the rule is/was.

I am too. There is no rule in sage community to decide what is a rule :-)

People have varying preferences in SageMath and we do not enforce any particular code style. I don't think this is a huge problem but I remember that it's confusing when you are working on your first contributions that it's a bit unclear which example to follow.

I think the part after the "--" should be treated the same as the text body preceding the "INPUT". One can see how to type the default value from the function header.

I agree with this.

Actually, in my REPL this does not work reliably. For functions I see the signature with ? but for methods it's not always shown:

sage: ZZ.coerce_map_from?
Docstring:
   Return a "Map" object to coerce from "S" to "self" if one exists,
   or "None" if no such coercion exists.

   EXAMPLES:

   By https://github.com/sagemath/sage/issues/12313, a special kind of
   weak key dictionary is used to store coercion and conversion maps,
   namely "MonoDict". In that way, a memory leak was fixed that would
   occur in the following test:

      sage: import gc
      sage: _ = gc.collect()
      sage: K = GF(1<<55,'t')
      sage: for i in range(50):
      ....:   a = K.random_element()
      ....:   E = EllipticCurve(j=a)
      ....:   b = K.has_coerce_map_from(E)
      sage: _ = gc.collect()
      sage: len([x for x in gc.get_objects() if isinstance(x, type(E))])
      1
Init docstring: Initialize self.  See help(type(self)) for accurate signature.
File:           /usr/lib/python3.12/site-packages/sage/structure/parent.pyx
Type:           builtin_function_or_method

That's probably a separate problem. If the signature were always shown with the docstring, then I don't think it's so important to spell out the defaults in that technical form.

[...]
In this example,

       - ``p`` -- prime integer (default: `2`); coprime with `n`

we want to say (C) is recommended.

I see your point here. I had another look at the documentation, and arguably `1` looks bad from a typography point of view:

image

Ideally, limitations in the renderer should not inform how we write docstrings but I remember that in Wikipedia there were similar rules about not using LaTeX for something simple like a 1 since it changes the font and breaks the flow for the reader.

From that perspective, a simple plain default: 1 would be best here. I find the output of default: ``1`` also acceptable:

image

@gmou3
Copy link
Contributor

gmou3 commented Jun 1, 2024

Your argumentation about simple values not being LaTeXed sounds good to me. Indeed it is ugly otherwise.
So (default: 1) is probably the best choice, with single backticks being reserved for more complicated math (as should also be the case in other docstring text).

Maybe we should add a guiding example for every usual scenario. What about strings for example? A) string, B) "string", C) "string", or D) 'string'? I think one often encounters string, which doesn't seem right.

@gmou3
Copy link
Contributor

gmou3 commented Jun 1, 2024

Actually, in my REPL this does not work reliably. For functions I see the signature with ? but for methods it's not always shown

It works with ?? however (which is supposed to yield the source code).

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jun 2, 2024

I am too. There is no rule in sage community to decide what is a rule :-)

People have varying preferences in SageMath and we do not enforce any particular code style. I don't think this is a huge problem but I remember that it's confusing when you are working on your first contributions that it's a bit unclear which example to follow.

I fully agree.

Actually, in my REPL this does not work reliably. For functions I see the signature with ? but for methods it's not always shown ...

Unfortunately, cython method does not always show the signature. This is an old issue: #26254

In this regard, there is subtle difference between our definitions of "default value". For me, default value is the value that the function assumes for an input when the user does not give a specific value. On the other hand, the default value in the function signature is always a python object (code), as you said. In my view, the former default value is what the latter default value represents when it is mathematical.

... and arguably `1` looks bad from a typography point of view:

Perhaps yes. But we are writing math $n$ or $x$ in the docstring. Why default value is an exception?

If there is not much math in the function or that 1 and other integers are only values, then simple 1 is preferable. This is all what the author should decide. We cannot recommend one style unilaterally.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jun 2, 2024

What about strings for example? A) string, B) "string", C) "string", or D) 'string'? I think one often encounters string, which doesn't seem right.

``'string'``

is what @mkoeppe writes (in his extensive work on cleaning doctests). I fully agree with this style.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jun 2, 2024

Recently I saw

(default: empty list)

I think this is perfectly okay. This is a good example where the default value is not the literal value in the function signature.

@gmou3
Copy link
Contributor

gmou3 commented Jun 2, 2024

I propose the addition of a line such as

- ``var`` -- string (default: ``'lambda'``)

The 1 or `1` point is somewhat subtle. Can we maybe leave it at `1` for consistency (because it is something mathematical) and have a LaTeX rule or package (mathastext?) do the typography work for us? That is, simplifying very simple math like a natural number to plain text. (The latter to be done in another PR if someone wishes to pursue it.)

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jun 2, 2024

I propose the addition of a line such as

- ``var`` -- string (default: ``'lambda'``)

OK. I will do it.

The 1 or 1 point is somewhat subtle. Can we maybe leave it at 1 for consistency (because it is something mathematical) ...

If you mean to do nothing about existing instances of 1 or 0 as default values, I do agree. As it is subtle, they should be treated case by case.

... simplifying very simple math like a natural number to plain text. (The latter to be done in another PR if someone wishes to pursue it.)

We don't do that when we write a paper. There "1" is usually math and written $1$ in latex. On the other hand, "1" in "Section 1" is not math. I think we should follow the same rule in our docstrings. If a python function is not mathematical, and expects an integer as input, then it is ``1``.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jun 2, 2024

I propose the addition of a line such as

- ``var`` -- string (default: ``'lambda'``)

Done.

Copy link
Contributor

@gmou3 gmou3 left a comment

Choose a reason for hiding this comment

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

Thanks.

vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 4, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

which I think are already established.  But it is good to recommend
preferred styles to keep docstrings in consistent styles.

motivated by the comment:
sagemath#37821 (comment)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

sagemath#38068 (merged here)
    
URL: sagemath#38117
Reported by: Kwankyu Lee
Reviewer(s): gmou3, Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 5, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

which I think are already established.  But it is good to recommend
preferred styles to keep docstrings in consistent styles.

motivated by the comment:
sagemath#37821 (comment)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

sagemath#38068 (merged here)
    
URL: sagemath#38117
Reported by: Kwankyu Lee
Reviewer(s): gmou3, Kwankyu Lee
@vbraun vbraun merged commit e6c4f20 into sagemath:develop Jun 9, 2024
21 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Jun 9, 2024
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.

5 participants