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.quadratic_forms: Modularization fixes for imports #35305

Merged
merged 40 commits into from
Apr 23, 2023

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Mar 18, 2023

📚 Description

We make sage.quadratic_forms ready for modularized distributions by:

  • moving imports from some packages, such as sage.rings.padics.factory, into methods
  • using lazy_import to provide the methods of QuadraticForm implemented in separate modules
  • adding # optional annotations to doctests

Also some improvements to the code style of doctests and the Sphinx markup of some docstrings have been made.

This has been and can be tested by merging into:

Resolves #35243

Part of:

📝 Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@codecov-commenter
Copy link

codecov-commenter commented Mar 19, 2023

Codecov Report

Patch coverage: 91.08% and project coverage change: -0.30 ⚠️

Comparison is base (c00e6c2) 88.62% compared to head (ff2df56) 88.32%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35305      +/-   ##
===========================================
- Coverage    88.62%   88.32%   -0.30%     
===========================================
  Files         2148     2145       -3     
  Lines       398855   398750     -105     
===========================================
- Hits        353480   352193    -1287     
- Misses       45375    46557    +1182     
Impacted Files Coverage Δ
src/sage/arith/all.py 100.00% <ø> (ø)
src/sage/modules/fg_pid/fgp_module.py 94.16% <ø> (ø)
src/sage/quadratic_forms/constructions.py 37.03% <ø> (-51.86%) ⬇️
src/sage/quadratic_forms/extras.py 86.44% <ø> (-8.48%) ⬇️
src/sage/quadratic_forms/qfsolve.py 68.88% <ø> (-31.12%) ⬇️
src/sage/quadratic_forms/quadratic_form__genus.py 80.00% <ø> (-7.50%) ⬇️
..._forms/quadratic_form__local_density_congruence.py 84.12% <ø> (ø)
..._forms/quadratic_form__local_density_interfaces.py 80.00% <ø> (ø)
...quadratic_form__local_representation_conditions.py 40.19% <ø> (-44.05%) ⬇️
src/sage/quadratic_forms/ternary_qf.py 66.60% <ø> (-0.59%) ⬇️
... and 29 more

... and 41 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mkoeppe mkoeppe requested a review from tornaria March 21, 2023 02:30
@mkoeppe mkoeppe force-pushed the sage_quadratic_forms_modularization branch from b9b1b68 to db14049 Compare March 27, 2023 04:58
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 31, 2023

#35166 makes is_fundamental_discriminant a method of Integer, should rebase on top of this

@mkoeppe mkoeppe force-pushed the sage_quadratic_forms_modularization branch from db14049 to 2517312 Compare March 31, 2023 18:25
@kwankyu
Copy link
Collaborator

kwankyu commented Apr 13, 2023

Otherwise, LGTM.

@mkoeppe mkoeppe force-pushed the sage_quadratic_forms_modularization branch from 55d3560 to d91a795 Compare April 13, 2023 19:40
@kwankyu
Copy link
Collaborator

kwankyu commented Apr 13, 2023

I looked through the rendered documentation. There are many math formulas with asterisks * representing multiplication. It seems they are not new. So you don't have to remove them. But it would be nice if you do it while we are here.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 14, 2023

Yes, these don't look very pretty. But I thought the intention is that "*" shows up in the text version (I don't know if that would be important.)

In sage.misc.sagedoc.math_substitutes (see also #35493), one can see the mapping (r'\\cdot', ' *').
LaTeX also has the discretionary multiplication symbol \*. Wondering if we should create a mapping (r'\\*', '*') -- such multiplication signs would then be displayed as * in the text help, implicit multiplication (no symbol) in the Sphinx HTML, and as implicit multiplication (no symbol unless at the end of a print line) in Sphinx PDF.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 14, 2023

I have changed * to \cdot

@mkoeppe mkoeppe force-pushed the sage_quadratic_forms_modularization branch from ed1c946 to a937a04 Compare April 14, 2023 04:50
@kwankyu
Copy link
Collaborator

kwankyu commented Apr 14, 2023

In sage.misc.sagedoc.math_substitutes (see also #35493), one can see the mapping (r'\\cdot', ' *'). LaTeX also has the discretionary multiplication symbol \*. Wondering if we should create a mapping (r'\\*', '*') -- such multiplication signs would then be displayed as * in the text help, implicit multiplication (no symbol) in the Sphinx HTML, and as implicit multiplication (no symbol unless at the end of a print line) in Sphinx PDF.

For me, \cdot means inner product, and is distinct from multiplication sign. But I didn't investigate if \cdot is used for inner product or multiplication (or perhaps for both) in sage docstrings.

For multiplication, I like the mapping (r'\\*', '*') that you suggested.

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 14, 2023

I have changed * to \cdot

I am not sure since I don't read \cdot as multiplication, except cases like $2 \cdot 3$. (I never doubted that I belong to the majority, but now I do :-)

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 14, 2023

But I didn't investigate if \cdot is used for inner product or multiplication (or perhaps for both) in sage docstrings.

I did. \cdot is used in many cases for multiplication in sage.

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 14, 2023

I have changed * to \cdot

Okay now.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 14, 2023

git grep 'cdot[^s]' shows lots of uses that look like multiplication (of all kinds of objects) to me.
But I'll try the \* tomorrow.

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 14, 2023

I am okay with either way.

@github-actions
Copy link

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 14, 2023

OK, then I'll leave experimentation with \* for another ticket, probably #35493.

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 for fixes. LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 14, 2023

Thanks very much for reviewing!

@vbraun vbraun merged commit 8bcce63 into sagemath:develop Apr 23, 2023
@mkoeppe mkoeppe added this to the sage-10.0 milestone Apr 23, 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.

sage.quadratic_forms: Remove module-level imports from number_fields, sage.symbolic, sage.functions
5 participants