Codecov Report

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

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

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

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

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

See

sage: L = IntegralLattice("D4")
sage: type(L)
<class 'sage.modules.free_quadratic_module_integer_symmetric.FreeQuadraticModule_integer_symmetric_with_category'>

and note that pari appears 119 times in the files in the directory sage/quadratic_forms/. As I see it, the quadratic forms module is a number theory subject and pretty dependent on integral lattices and pari.

So I think the distribution package that installs the quadratic forms module should also install the integral lattices module and the pari lib (which I want to hide as implementation details), getting rid of the # optional - M tags.

@yyyyx4 @tornaria

So I think the distribution package that installs the quadratic forms module should also install the integral lattices module and the pari lib (which I want to hide as implementation details), getting rid of the # optional - M tags.

• pari : seems clear, since a lot of functionality for the sage.quadratic_forms package is dependent on pari
• IntegralLattices, I don't know, never used it. It might be the case that IntegralLattice depends on sage.quadratic_forms in an essential way, but the other way around, in principle not (sage.quadratic_forms was mostly written in 2009 and IntegralLattice is newer I think), although the two might be more interlinked than I know and that may be a good thing (I'm actually quite happy to see this IntegralLattice class).

In any case, if the goal is to minimize "external" dependencies, I wouldn't worry too much about what packages are bundled together as long as they have the same external dependencies.

a. if package X uses external "pari" (say) the question is whether X has any utility at all when "pari" is not installed.
b. if packages X and Y both depend on the same external packages, there is no harm in bundling them together.

Whether sage.quadratic_forms and pari satisfy (a) or not and whether sage.quadratic_forms and IntegralLattices satisfy (b) or not, it's left as an exercise.

Anyway, I couldn't care less how this is done as long as I can still build sagelib as a whole (it takes me ~2m to build and ~3m to do a full test from gh source) and I'm not forced to use bundled external dependencies. Try not to choke with the bikeshed paint 🙄

In #35095 I am building the distribution sagemath-polyhedra, and I want to ship portions of sage.quadratic_forms and modules (yes, including IntegerLattice). Yes, quadratic forms is a number theory subject; but for polyhedra / lattice points in polyhedra we do need some LLL and other reductions. PARI is only one of the implementation options for that -- we also have fpylll, which is a much smaller and more well-behaved implementation.

I have already figured out what parts of sage.quadratic_forms depend on PARI. See
https://github.com/sagemath/sage/pull/35095/files#diff-f4c7c1beea87ae44c426de0a55459daef63b75b701b72bc136d88f4e7c9285cfR104

I wouldn't worry too much about what packages are bundled together as long as they have the same external dependencies.

That's right; the dependencies on compiled packages are the hard constraints. I am trying to make the modularization not much more fine-grained than what is dictated by these constraints. So yes, probably everything that (compile-time/load-time) depends on PARI will be shipped by one distribution package sagemath-pari.

I have already figured out what parts of sage.quadratic_forms depend on PARI. See https://github.com/sagemath/sage/pull/35095/files#diff-f4c7c1beea87ae44c426de0a55459daef63b75b701b72bc136d88f4e7c9285cfR104

Yes. They are a big portion of sage.quadratic_forms.

In #35095 I am building the distribution sagemath-polyhedra, and I want to ship portions of sage.quadratic_forms and modules (yes, including IntegerLattice). Yes, quadratic forms is a number theory subject; but for polyhedra / lattice points in polyhedra we do need some LLL and other reductions. PARI is only one of the implementation options for that -- we also have fpylll, which is a much smaller and more well-behaved implementation.

I think that distributions in the top layer of the hierarchy should follow the mathematical disciplines, like your sagemath-polyhedra. Likewise, instead of sagemath-pari, we may imagine sagemath-quadratic-forms or sagemath-integral-lattices or big (too big) umbrella sagemath-number-theory. Any of these distributions would want to install all of sage.quadratic_forms, including the portions included to sagemath-polyhedra. Then the optional tags tailored for sagemath-polyhedra seem inappropriate for the distribution, say, sagemath-quadratic-forms. No?

How would sagemath-quadratic-forms be organized? Would it be depend on sagemath-polyhedra?

How would sagemath-quadratic-forms be organized? Would it be depend on sagemath-polyhedra?

Below sagemath-polyhedra we would probably create something like sagemath-modules. That's easy refactoring after #35095 is done.
Both sagemath-polyhedra and sagemath-quadratic-forms would then be able to depend on that.

Otherwise, LGTM.

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.

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.

I have changed * to \cdot

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.

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 :-)

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.

I have changed * to \cdot

Okay now.

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.

I am okay with either way.

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

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

Thanks very much for reviewing!