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.

Sign up for GitHub

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

Jump to bottom

sage.categories: Add # optional for modularization; reformat doctests #35422

Merged
merged 24 commits into from
May 22, 2023
Merged

sage.categories: Add # optional for modularization; reformat doctests #35422

merged 24 commits into from
May 22, 2023

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Apr 2, 2023 •
edited
Loading

📚 Description

Adding doctest tags # optional - sage.rings.finite_rings, ...number_field, ...padics etc. for modularization purposes.

Also:

  • some coding style fixes in the doctests (such as adding spaces around some operators and after commas, following PEP 8)
  • improved the readability of doctests in the HTML format by breaking long lines to avoid having to scroll horizontally

This is:

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • 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 accordingly.

⌛ Dependencies

@mkoeppe mkoeppe self-assigned this Apr 2, 2023
@mkoeppe mkoeppe requested a review from tscrim April 5, 2023 04:53
@mkoeppe mkoeppe force-pushed the sage_categories_reformat_doctests branch from 978fe20 to 7cb61c9 Compare April 16, 2023 00:17
@mkoeppe mkoeppe requested a review from tornaria April 16, 2023 00:18
tornaria
tornaria reviewed Apr 16, 2023
View reviewed changes
src/sage/categories/coxeter_groups.py
Comment on lines +796 to +798
sage: W = CoxeterGroup('B3', implementation='coxeter3') # optional - coxeter3
sage: b3_cells = W.kazhdan_lusztig_cells('two-sided') # optional - coxeter3
sage: len(b3_cells) # optional - coxeter3
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the convention here, if not column 88?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an actual optional package; they should be aligned so that the tag is visible in full in the formatted doc.
My editor macro aligns these at column 64; but I think it's too hard (and not necessary) to make it globally consistent.

tornaria
tornaria reviewed Apr 16, 2023
View reviewed changes
src/sage/categories/enumerated_sets.py
@@ -494,7 +495,7 @@ def tuple(self):

EXAMPLES::

sage: (GF(3)^2).tuple()
sage: (GF(3)^2).tuple() # optional - sage.libs.pari
((0, 0), (1, 0), (2, 0), (0, 1), (1, 1), (2, 1), (0, 2), (1, 2), (2, 2))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe split this result so it fits in 80 columns? There are a few more below, some even a bit longer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Column 80 plays no role in my undocumented style guide. I let the output run to column 86 regularly, and sporadically to column 96. (After that, horizontal scrolling is necessary.)

tornaria
tornaria reviewed Apr 16, 2023
View reviewed changes
src/sage/categories/filtered_modules_with_basis.py
Comment on lines +203 to +206
Lazy family (Term map
from Partitions
to An example of a graded module with basis: the free module
on partitions over Integer Ring(i))_{i in Partitions of the integer 4}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe split one more line so it gets under 80 columns?
Let me try:

Suggested change
Lazy family (Term map
from Partitions
to An example of a graded module with basis: the free module
on partitions over Integer Ring(i))_{i in Partitions of the integer 4}
Lazy family (Term map
from Partitions
to An example of a graded module with basis:
the free module on partitions over Integer Ring(i)
)_{i in Partitions of the integer 4}

Not sure if that's actually ok... Your version is not so bad either, do as it pleases you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My version runs to column 91, which I think is fine because there are no # optional tags around at column 88

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally in reformatting doctest output, I don't introduce whitespace where no whitespace was before

tornaria
tornaria reviewed Apr 16, 2023
View reviewed changes
src/sage/categories/finite_complex_reflection_groups.py
@@ -56,15 +56,15 @@ class FiniteComplexReflectionGroups(CategoryWithAxiom):
sage: W = ComplexReflectionGroups().Finite().example(); W # optional - gap3
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems the # optional - gap3 could all go in the same column for the whole file except for this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is at 72 (same as earlier up in the file). There are some occurrences around lines 900-1000 that I aligned at column 80 (rightmost place where the whole tag is visible) because the lines are longer. But in docstrings that have column-88 aligned tags, it has to be aligned farther to the left.

tornaria
tornaria reviewed Apr 16, 2023
View reviewed changes
src/sage/categories/functor.pyx Outdated
@@ -477,7 +487,7 @@ class ForgetfulFunctor_generic(Functor):
"""
TESTS::

sage: F = ForgetfulFunctor(FiniteFields(),Fields())
sage: F = ForgetfulFunctor(FiniteFields(), Fields())
sage: F #indirect doctest
The forgetful functor from Category of finite enumerated fields to Category of fields
Copy link
Contributor

Choose a reason for hiding this comment

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

want to split this output line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In TESTS blocks, I have been less strict with the formatting because by default it is not shown in the formatted documentation.
Same for docstrings of private/special methods unless they appear in the formatted documentation

Matthias Koeppe and others added 12 commits April 15, 2023 19:50
@mkoeppe
src/sage/categories/finite_dimensional_nilpotent_lie_algebras_with_ba…
f985b6c 
…sis.py: Align # optional
@mkoeppe
src/sage/categories: Re-align # optional
3d52da4
@mkoeppe @tornaria
Update src/sage/categories/lambda_bracket_algebras_with_basis.py
d1dd594 
Co-authored-by: Gonzalo Tornaría <tornaria@gmail.com>
@mkoeppe @tornaria
Update src/sage/categories/lambda_bracket_algebras_with_basis.py
7d114f9 
Co-authored-by: Gonzalo Tornaría <tornaria@gmail.com>
@mkoeppe
src/sage/categories: Re-align # optional
2d0fb65
@mkoeppe
src/sage/categories: Re-align # optional
897e21a
@mkoeppe
src/sage/categories: Re-align # optional
f911b14
@mkoeppe @tornaria
Update src/sage/categories/category.py
1595829 
Co-authored-by: Gonzalo Tornaría <tornaria@gmail.com>
@mkoeppe
src/sage/categories: Re-align # optional
260b83b
@mkoeppe
Fixup
21952a1
@mkoeppe
Merge tag '10.0.rc0' into sage_categories_reformat_doctests
59ab512 
SageMath version 10.0.rc0, Release Date: 2023-04-23
@mkoeppe
sage.categories: More # optional
67b892b
kwankyu
kwankyu reviewed Apr 27, 2023
View reviewed changes
src/sage/categories/finite_dimensional_algebras_with_basis.py
sage: from sage.monoids.hecke_monoid import HeckeMonoid # optional - sage.groups
sage: A = HeckeMonoid(SymmetricGroup(4)).algebra(QQ) # optional - sage.groups sage.modules
sage: idempotents = A.orthogonal_idempotents_central_mod_radical() # optional - sage.groups sage.modules sage.rings.number_field
sage: A.is_identity_decomposition_into_orthogonal_idempotents(idempotents) # optional - sage.groups sage.modules sage.rings.number_field
True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not align here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This super long identifier somehow stopped me from finding a better looking solution

kwankyu
kwankyu reviewed Apr 27, 2023
View reviewed changes
src/sage/categories/lie_algebras.py
sage: L = LieAlgebras(QQ).example()
sage: L._test_distributivity()
sage: L = LieAlgebras(QQ).example() # optional - sage.combinat sage.modules
sage: L._test_distributivity() # optional - sage.combinat sage.modules

Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this too much far to the right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm. I see. This is aligned with other rows below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And it's in a TESTS block of a private function, so it won't show in the HTML

kwankyu
kwankyu reviewed Apr 27, 2023
View reviewed changes
src/sage/categories/vector_spaces.py
sage: W.dimension()
sage: M = FreeModule(FiniteField(19), 100) # optional - sage.libs.pari sage.modules
sage: W = M.submodule([M.gen(50)]) # optional - sage.libs.pari sage.modules
sage: W.dimension() # optional - sage.libs.pari sage.modules
1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this sage.rings.finite_rings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, thanks.

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 27, 2023

Otherwise, LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 27, 2023

Thanks for reviewing!

kwankyu
kwankyu approved these changes Apr 28, 2023
View reviewed changes
Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

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

Thanks. Then this is good to go.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 28, 2023

Thank you!

@mkoeppe mkoeppe force-pushed the sage_categories_reformat_doctests branch from ef851ce to 4a7c0c2 Compare May 15, 2023 23:24
@github-actions
Copy link

Documentation preview for this PR is ready! 🎉
Built with commit: e02b79e

@vbraun vbraun merged commit b378119 into sagemath:develop May 22, 2023
@mkoeppe mkoeppe added this to the sage-10.1 milestone May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tornaria tornaria tornaria left review comments

@kwankyu kwankyu kwankyu approved these changes

@tscrim tscrim Awaiting requested review from tscrim

Assignees

@mkoeppe mkoeppe

Labels
c: categories c: documentation t: refactoring t: tests
Projects
None yet
Milestone
sage-10.1
Development

Successfully merging this pull request may close these issues.
4 participants
@mkoeppe @kwankyu @tornaria @vbraun