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: fix sparse docs "matrix" and "array" mixup #18898

Merged

Conversation

KatMistberg
Copy link
Contributor

Reference issue

Closes #18669.

What does this implement/fix?

Currently, the "Usage information" section of the docs of the sparse module still refers to the matrix interface while nowadays the array interface is preferred. Additionally, the docs of the individual array and matrix classes (as well as their method docstrings) contain a mix of "matrix" and "array".

This PR replaces the relevant instances of "matrix" with "array" under the "Usage information" section, and for the class docs, it makes it so that the array classes use the word "array" while the matrix classes use the word "matrix". Because of limitations of the Python doc system, the method docstrings are shared between the array and matrix classes, so this PR replaces all relevant instances of "array" and "matrix" in the method docstrings with "array/matrix". When the matrix interface is deprecated in the future, this can be changed to simply "array".

Additional information

@KatMistberg
Copy link
Contributor Author

Sorry, I shouldn't have merged main into this branch at the end there. Hopefully it isn't a big problem.

@perimosocordiae
Copy link
Member

We discussed this PR at the last meeting of the Sparse Arrays working group (see https://scientific-python.org/calendars/ if you'd like to attend).

The consensus of that discussion, if I recall correctly, was:

  1. This is an improvement, for sure. Thank you for the PR!
  2. It seems like the growing stack of string transformations we're applying are becoming unwieldy, so we're thinking of accepting some duplication and writing each class/method docstring separately for array and matrix base classes.

If you'd prefer to merge this in the current format, we can do a mechanical translation to achieve (2) after. That said, if you'd like to take that effort on as well in this PR, that's ok too.

@KatMistberg
Copy link
Contributor Author

Thanks for reviewing my PR!

Regarding having separate docstrings for arrays and matrices, while it would work well for the class docstrings, I'm not sure it would work as well for method docstrings. A lot of the methods of the array (e.g. bsr_array) and matrix (bsr_matrix) classes are defined not in the base class (_bsr_base), but in the ancestors of the base class (_cs_matrix, _data_matrix, etc). This means that the easiest solution would be, I think, for each (public) method in the array and matrix classes, to add a wrapper method that calls the inherited method of the same name, and having a custom docstring for the wrapper method. To me, this still seems pretty messy and does not seem to be better than having "array/matrix" for all method docstrings (as in the current PR), but that could just be me.

I hope I understood correctly what you meant with (2). I'll also attend the upcoming Sparse Array meeting so we can also discuss this further then. I think it makes sense to resolve the above in this PR as well, so I'll work on it and let's not merge it yet.

@KatMistberg KatMistberg force-pushed the 18669-fix-sparse-docs-matrix-array branch from 912b019 to 91fb866 Compare July 27, 2023 16:21
@KatMistberg
Copy link
Contributor Author

As discussed in the Sparse array meeting on July 21, it has been decided to move the definition of the sparse class docs from the base classes to the array/matrix classes for this PR, and the method docs can be dealt with later. This also means that the string replacement is no longer needed. This last commit achieves this, so if it is approved the PR can be merged.

Copy link
Contributor

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

Thank you, @KatMistberg.

I think we might change "array/matrix" for something else like "array or matrix" or just keep "array". What do you think?

Here are a few comments and suggestions.

scipy/sparse/__init__.py Outdated Show resolved Hide resolved
scipy/sparse/_base.py Outdated Show resolved Hide resolved
scipy/sparse/_base.py Outdated Show resolved Hide resolved
scipy/sparse/_base.py Outdated Show resolved Hide resolved
scipy/sparse/_base.py Show resolved Hide resolved
scipy/sparse/_csc.py Outdated Show resolved Hide resolved
scipy/sparse/_coo.py Outdated Show resolved Hide resolved
scipy/sparse/_coo.py Outdated Show resolved Hide resolved
scipy/sparse/_csr.py Outdated Show resolved Hide resolved
scipy/sparse/_csr.py Outdated Show resolved Hide resolved
@KatMistberg KatMistberg force-pushed the 18669-fix-sparse-docs-matrix-array branch from 7986f90 to ce7595b Compare August 27, 2023 07:06
Kat Mistberg added 8 commits September 3, 2023 10:24
Before, the docs for sparse arrays/matrices were written
for the array classes and "sparse array" was replaced with
"sparse matrix" to get the matrix class doc, but that did
not cover all strings that need to be replaced. Now, the
base docstrings have strings e.g. "{array|matrix}" that are
replaced with "array" or "matrix" to get the array or
matrix docs. Docstrings of the different sparse formats
are also made more consistent. See scipy#18669.
Under "Usage information" in the main sparse doc page, the
matrix interface is still used; this commit switches it
all to the (recommended) array interface. Doc code
formatting is also improved. See scipy#18669.
Prior to this, the method docstrings of sparse classes
(and their parent classes) were a mix of "array"s and
"matrix"s. This commit makes this consistent by changing
all relevant instances of "array" and "matrix" to
"array/matrix". When sparse matrices are deprecated,
change all instances of "array/matrix" to "array".
See scipy#18669.
Previously, the class docs of the sparse array/matrix
classes were defined in the base class, and then a string
replacement was used to obtain the array and matrix class
docs. To allow for more divergent array and matrix class
docs, it has been decided to simply define the docstrings
in the array/matrix classes, and this commit does that.
See scipy#18669.
@KatMistberg KatMistberg force-pushed the 18669-fix-sparse-docs-matrix-array branch from ce7595b to f69def3 Compare September 3, 2023 08:26
@perimosocordiae perimosocordiae merged commit d781501 into scipy:main Sep 11, 2023
24 checks passed
@perimosocordiae
Copy link
Member

Merged, thanks @KatMistberg for sticking with this PR through all the merge conflicts and changes!

I suspect that we'll find minor things to tweak in the docs here and there, but I don't want to delay getting this in any longer than it already has. Now that the big change is in, we can iterate with smaller docfix PRs more rapidly as needed.

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.

DOC: sparse docs "matrix" and "array" mixup
4 participants