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

Doc: Reorganize math module documentation #126337

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

Conversation

Nodd
Copy link
Contributor

@Nodd Nodd commented Nov 2, 2024

Following #125810, here is a proposal to reorganize the math documentation. I took into account the comments from the previous PRs : #125810 (comment) #125810 (review) #125810 (comment) and tried to improve from here. As suggested in a comment, I used https://en.cppreference.com/w/c/numeric/math as reference.

One thing I'm not fond of is the duplication of the paragraph about the return values, now that frexp and modf are in different sections. Note that modf is also cited in another paragraph in Basic floating point operations.

Another question I have is, should the functions be listed in alphabetic order, or in a logical order ? For example, the trigonometric functions could start with sin and cos instead of acos and asin, or gcd and lcm could be listed together. Same with sqrt and cbrt.

Direct link to the updated math module documentation


📚 Documentation preview 📚: https://cpython-previews--126337.org.readthedocs.build/

@bedevere-app bedevere-app bot added docs Documentation in the Doc dir skip news awaiting review labels Nov 2, 2024
Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Doc/library/math.rst Outdated Show resolved Hide resolved
Doc/library/math.rst Outdated Show resolved Hide resolved
@Nodd
Copy link
Contributor Author

Nodd commented Nov 2, 2024

Thanks @picnixz for the review, I included your modifications.

Doc/library/math.rst Outdated Show resolved Hide resolved
Doc/library/math.rst Show resolved Hide resolved
@skirpichev skirpichev self-requested a review November 3, 2024 05:51
@Nodd Nodd marked this pull request as draft November 6, 2024 02:47
@Nodd Nodd marked this pull request as ready for review November 6, 2024 02:55
@Nodd
Copy link
Contributor Author

Nodd commented Nov 6, 2024

Since this PR touches the whole file, merging and fixing the conflict was a pain... If this new layout is fine, can it be merged before another conflict arises? 😅

@vstinner
Copy link
Member

vstinner commented Nov 7, 2024

Overall, I like the new math doc organization. Nice work.

Special functions

I'm not sure how special are these functions. Maybe just say "Other functions"?

@picnixz
Copy link
Contributor

picnixz commented Nov 7, 2024

We had a brief discussion on this in #126337 (comment) (see also https://mathworld.wolfram.com/SpecialFunction.html).

Now, not everyone knows about the "special functions" terminology which is first informal (there is no formal definition of what a "special function", it's just that we grouped some functions under the category of "Special functions") and second, it allows us to add whatever miscelleanous functions we want to add (although Sergey hinted that we probably won't add more than what the C standard does). So I think "Miscellaneous functions" or "Other functions" is also a good title.

@Nodd
Copy link
Contributor Author

Nodd commented Nov 7, 2024

@vstinner Thank you for your kind words.

About special functions, I proposed a change, see this comment thread. I learned that those are actually special.

@Nodd
Copy link
Contributor Author

Nodd commented Nov 7, 2024

After thinking a bit more about it, one advantage of "Special function" is that it has a meaning to the specialists, and is no less informative than "Miscellaneous functions" or "Other functions" to other people. Those titles are too generic to actually mean anything.

My initial proposal was to be explicit by using "Error and gamma functions". This title could be changed if another function is added in the future. In the end, I simply went with status quo wins.

@skirpichev
Copy link
Member

My initial proposal was to be explicit by using "Error and gamma functions".

This is a best variant, IMO. It's aligned with the C standard.

More generic title (like "Special functions") opens also the door for enhancement requests "lets add also this and that special functions".

Doc/library/math.rst Outdated Show resolved Hide resolved
Doc/library/math.rst Outdated Show resolved Hide resolved
Doc/library/math.rst Outdated Show resolved Hide resolved
@Nodd
Copy link
Contributor Author

Nodd commented Nov 12, 2024

I resolved all the conversations.

@picnixz @skirpichev There is only the remaining question about "Special functions" vs "Error and gamma functions" vs something else. Who gets to decide?

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

I like the new categories. The change mostly moves functions, the few extra changes are acceptable (I recreated the PR manually to detect "extra" changes").

I'm fine with "Special functions" which is already used.


Module :mod:`cmath`
Complex number versions of many of these functions.

Copy link
Member

@skirpichev skirpichev Nov 14, 2024

Choose a reason for hiding this comment

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

One last nitpick. Lets not move this footer.

Edit: I meant the whole footer, not just the reference to cmath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had two things in mind when moving this block:

  • I find it not easily discoverable, I wasn't expecting another general paragraph after the API listing
  • In the page layout, it effectively appears in the Constants section, which I find confusing.

I propose to put it back at the end as you asked, and to create a new section named Implementation detail. That would fix both of my concerns.

Copy link
Member

Choose a reason for hiding this comment

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

I find it not easily discoverable

It a good thing.

In the page layout, it effectively appears in the Constants section, which I find confusing.

I think that it's visually distinct from the sphinx docs of constants. No need for extra section.

Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

LGTM (modulo Sergey's comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

4 participants