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

sage.matroids: Modularization fixes #35719

Merged
merged 14 commits into from
Jun 21, 2023

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Jun 4, 2023

📚 Description

Fixes for imports and adding # optional.

Also some # optional updates in other modules.

📝 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

@tobiasdiez
Copy link
Contributor

Would it be possible to filter out all these optional tags from the docs? Its a bit annoying and they don't really add anything.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 4, 2023

Would it be possible to filter out all these optional tags from the docs?

That would be very useful for the PDF documentation because in that format, the comments wrap around.

For HTML, I want to keep them.

Its a bit annoying

I have aligned these annotations so that only a little bit (# option....) is visible in order to minimize annoyance.

and they don't really add anything.

They do add important information for users of the modularized distributions: They can see in the documentation what examples can be run when this or that is installed but other things aren't, so that they can avoid trial and error.

Moreover, annotations that refer to libraries, such as # optional - sage.libs.singular, also give attribution to where the functionality of this example is implemented.

Here in the case of sage.matroids, the annotations also give the reader a rough idea how examples are constructed (from graphs? from linear algebra over finite fields? from schemes?), and which parts of the functionality are graph-theoretic in nature.

@tobiasdiez
Copy link
Contributor

Its a bit annoying

I have aligned these annotations so that only a little bit (# option....) is visible in order to minimize annoyance.

This is not really working for me.
image

and they don't really add anything.

They do add important information for users of the modularized distributions: They can see in the documentation what examples can be run when this or that is installed but other things aren't, so that they can avoid trial and error.

In my opinion this "# optional" tag is not self-explanatory enough to provide the information you describe. I agree that users might profit from this, but currently its too opaque. I propose we strip them for now, and if you think the information is worth it parse the optional tags and add a tiny notice "This example requires xyz and abc" at the end, with hyperlinks to xyz and abc.

(sorry for this somewhat off-topic discussion, but I think the representation of these optional tags should be discussed before adding even more to the docs.)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 5, 2023

In my opinion this "# optional" tag is not self-explanatory enough to provide the information you describe. I agree that users might profit from this, but currently its too opaque.

Sure, documentation will be needed. It will be added in the documentation of sage.features and in the READMEs of the distributions. (See https://github.com/sagemath/sage/pull/35095/files for the new distributions.)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 5, 2023

I propose we strip them for now, and if you think the information is worth it parse the optional tags and add a tiny notice

I don't plan to work on this

@tobiasdiez
Copy link
Contributor

Based on https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-trim_doctest_flags and https://github.com/sphinx-doc/sphinx/blob/d3c91f951255c6729a53e38c895ddc0af036b5b9/sphinx/transforms/post_transforms/code.py#L82-L125 this shouldn't be that hard. Maybe

from sphinx.ext import doctest as sphinx_doctest
sphinx_doctest.doctestopt_re = re.compile(r'#\s*(doctest|optional):.+$', re.MULTILINE)
trim_doctest_flags = True

or overwriting TrimDoctestFlagsTransform

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 6, 2023

Thanks, I've added this info to:

That we are adding these annotations is already documented in https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#preparing-doctests

There's nothing that "needs work" here.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 9, 2023

In my opinion this "# optional" tag is not self-explanatory enough to provide the information you describe. I agree that users might profit from this, but currently its too opaque.

Sure, documentation will be needed. It will be added in the documentation of sage.features

This is happening in #35749

@mkoeppe mkoeppe requested a review from dcoudert June 13, 2023 16:14
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 16, 2023

The test failure in ell_rational_field is unrelated.

Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

a few comments.

src/sage/dynamics/arithmetic_dynamics/projective_ds.py Outdated Show resolved Hide resolved
src/sage/matroids/linear_matroid.pyx Outdated Show resolved Hide resolved
src/sage/matroids/minor_matroid.py Outdated Show resolved Hide resolved
@github-actions
Copy link

Documentation preview for this PR (built with commit 4d507c7) is ready! 🎉

Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 17, 2023

Thank you! May I set to "positive review"?

@dcoudert
Copy link
Contributor

Done.

@vbraun vbraun merged commit bb0ea98 into sagemath:develop Jun 21, 2023
@mkoeppe mkoeppe added this to the sage-10.1 milestone Jun 21, 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