Skip to content

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Apr 2, 2023

📚 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

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.

@@ -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.)

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

@@ -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.

@@ -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 Köppe and others added 7 commits April 15, 2023 19:25
Co-authored-by: Gonzalo Tornaría <tornaria@gmail.com>
Co-authored-by: Gonzalo Tornaría <tornaria@gmail.com>
Co-authored-by: Gonzalo Tornaría <tornaria@gmail.com>
Co-authored-by: Gonzalo Tornaría <tornaria@gmail.com>
Co-authored-by: Gonzalo Tornaría <tornaria@gmail.com>
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

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

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!

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!

@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
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.

4 participants