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: Include np.long in arrays.scalars.rst #25030

Merged
merged 2 commits into from
Nov 2, 2023

Conversation

mtsokol
Copy link
Member

@mtsokol mtsokol commented Oct 30, 2023

Addresses #25015.

Hi @ngoldbaum, in this PR I updated arrays.scalars.rst so it includes np.long and updated diagrams. (the other one-line changes are fixes for some warnings that I found when building locally)

Copy link
Member

@ngoldbaum ngoldbaum left a 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 a desire to keep the .dia file and only modify that file and regenerate the png and PDF, from inside Dia see #23252 (comment). I'm able to open the file locally if I do sudo apt install dia then dia path/to/file.dia.

If that ends up being too cumbersome we could come back to how to handle the diagram files the numpy docs have in a few places but please give it a try first.

@mtsokol
Copy link
Member Author

mtsokol commented Oct 30, 2023

@ngoldbaum Sure! I restored&updated the .dia file and regenerated the new png/pdf diagram.

Nevertheless, I would argue that all diagrams could be migrated to e.g. Google Drawings or draw.io (both free), as the Dia is ancient and rather cumbersome to use. (I hope there isn't another reason to stick to it)

@ngoldbaum
Copy link
Member

ngoldbaum commented Oct 30, 2023

Nevertheless, I would argue that all diagrams could be migrated to e.g. Google Drawings or draw.io (both free), as the Dia is ancient and rather cumbersome to use.

Sure, I think work happened along these lines in #18501, but the diagrams never got updated . Ideally (I think) the source for the diagram would live in the docs. I'd argue it makes the most sense to use the built-in support in sphinx for the dot format, see e.g. #18501 (comment).

(I hope there isn't another reason to stick to it)

I'm pretty sure the only reason is inertia.

Copy link
Member

@ngoldbaum ngoldbaum left a comment

Choose a reason for hiding this comment

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

LGTM! @rossbar can you confirm what I'm saying to Mateusz about the diagrams is correct before I merge this?

@ngoldbaum
Copy link
Member

Let's just go ahead and merge this, we can fix doc issues in a followup.

@ngoldbaum ngoldbaum merged commit 3753282 into numpy:main Nov 2, 2023
58 checks passed
@mtsokol mtsokol deleted the numpy-long-doc-fix branch November 2, 2023 14:49
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.

2 participants