Fix construction functor for p-adic relaxed types. by tornaria · Pull Request #35441 · sagemath/sage

tornaria · 2023-04-05T16:42:39Z

The precision for relaxed p-adics has 3 components:

(default_prec, halting_prec, secure)

where the last one secure is a boolean defaulting to False.

However, the construction() method doesn't know about it:

sage: K = QpER(5, secure=True)
sage: K.construction(forbid_frac_field=True)
(Completion[5, prec=(20, 40)], Rational Field)
sage: R = ZpER(5, secure=True)
sage: R.construction()
(Completion[5, prec=(20, 40)], Integer Ring)

This has two undesired consequences for the change() method:

a. The secure attribute is not copied:

sage: K.is_secure()
True
sage: K.change().is_secure()
False
sage: R.is_secure()
True
sage: R.change().is_secure()
False

b. The check=False option is broken:

sage: K.change(check=False)
...
ValueError: not enough values to unpack (expected 3, got 2)
sage: R.change(check=False)
...
ValueError: not enough values to unpack (expected 3, got 2)

Fixing the construction() method solves both issues.

After this commit:

sage: K = QpER(5, secure=True)
sage: K.construction(forbid_frac_field=True)
(Completion[5, prec=(20, 40, True)], Rational Field)
sage: K.change().is_secure()
True
sage: K.change(check=False)
5-adic Field handled with relaxed arithmetics
sage: K.change(check=False).is_secure()
True

sage: R = ZpER(5, secure=True)
sage: R.construction()
(Completion[5, prec=(20, 40, True)], Integer Ring)
sage: R.change().is_secure()
True
sage: R.change(check=False)
5-adic Ring handled with relaxed arithmetics
sage: R.change(check=False).is_secure()
True

📝 Checklist

The title is concise, informative, and self-explanatory.
The description explains in detail what this PR is about.
I have created tests covering the changes.

roed314 · 2023-04-05T17:56:46Z

It looks like the linting errors were here. The PR looks fine otherwise.

roed314 · 2023-04-05T17:57:48Z

Alright, it seems like you need to merge #35418 here as well. I'll give both tickets positive review once tests pass.

mkoeppe · 2023-04-06T00:08:37Z

How come this is not caught by _test_construction? Is the TestSuite called at all for these parents?

tornaria · 2023-04-06T16:30:09Z

How come this is not caught by _test_construction? Is the TestSuite called at all for these parents?

TestSuite is called for QpER(7) and ZpER(7) which have the default secure=False attribute. The constructors for relaxed type do some acrobatics in ...padics.factory.get_key_base() to fix the parameter prec with this default (although this is not done when check=False so the constructor is less functional in that case ).

On sage 9.8:

sage: R = ZpER(7)
sage: TestSuite(R).run(skip=['_test_log','_test_matrix_smith'])
sage: R = ZpER(7, secure=True)
sage: TestSuite(R).run(skip=['_test_log','_test_matrix_smith'])
...
AssertionError: the object's construction does not recreate this object
------------------------------------------------------------
The following tests failed: _test_construction

The same thing works ok after this PR, I'll add running a doctest to run the testsuite as above.

tornaria · 2023-04-06T20:16:00Z

Unrelated (random) doctest failure, which I can reproduce locally with this particular random seed:

$ sage -t --random-seed=3462219213258779680603652694803022552 /usr/lib/python3.11/site-packages/sage/rings/polynomial/multi_polynomial.pyx
too many failed tests, not using stored timings
Running doctests with ID 2023-04-06-17-14-49-7f2db03c.
Running with SAGE_LOCAL='/usr' and SAGE_VENV='/usr'
Using --optional=pip,sage
Features to be detected: 4ti2,benzene,bliss,buckygen,conway_polynomials,csdp,cvxopt,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_cubic_hecke,database_jones_numfield,database_knotinfo,dvipng,gfan,graphviz,imagemagick,jupymake,kenzo,latte_int,lrslib,mcqd,meataxe,msolve,nauty,palp,pandoc,pdf2svg,pdftocairo,phitigra,plantri,polytopes_db,polytopes_db_4d,pynormaliz,python_igraph,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.groups,sage.misc.cython,sage.plot,sage.rings.number_field,sage.rings.padics,sage.rings.real_double,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,sphinx,tdlib
Doctesting 1 file.
sage -t --random-seed=3462219213258779680603652694803022552 /usr/lib/python3.11/site-packages/sage/rings/polynomial/multi_polynomial.pyx
**********************************************************************
File "/usr/lib/python3.11/site-packages/sage/rings/polynomial/multi_polynomial.pyx", line 1390, in sage.rings.polynomial.multi_polynomial.MPolynomial.sylvester_matrix
Failed example:
    f.sylvester_matrix(g, x).determinant() == f.resultant(g, x)
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib/python3.11/site-packages/sage/doctest/forker.py", line 695, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib/python3.11/site-packages/sage/doctest/forker.py", line 1093, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.polynomial.multi_polynomial.MPolynomial.sylvester_matrix[9]>", line 1, in <module>
        f.sylvester_matrix(g, x).determinant() == f.resultant(g, x)
        ^^^^^^^^^^^^^^^^^^^^^^^^
      File "sage/rings/polynomial/multi_polynomial.pyx", line 1474, in sage.rings.polynomial.multi_polynomial.MPolynomial.sylvester_matrix (build/cythonized/sage/rings/polynomial/multi_polynomial.c:17071)
        raise ValueError("The Sylvester matrix is not defined for zero polynomials")
    ValueError: The Sylvester matrix is not defined for zero polynomials
**********************************************************************
1 item had failures:
   1 of  36 in sage.rings.polynomial.multi_polynomial.MPolynomial.sylvester_matrix
    [577 tests, 1 failure, 3.72 s]
----------------------------------------------------------------------
sage -t --random-seed=3462219213258779680603652694803022552 /usr/lib/python3.11/site-packages/sage/rings/polynomial/multi_polynomial.pyx  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 8.8 seconds
    cpu time: 3.5 seconds
    cumulative wall time: 3.7 seconds
Features detected for doctesting:

tornaria · 2023-04-06T20:20:59Z

@roed314 all tests pass now here and in #35442.

Except for:

the deploy documentation (Fix documentation deployment #35356 and merging that one here won't fix it)
the bulid & test check here has a single unrelated failure as documented above

For the failure, I'll have a look and submit a PR if it's trivial to fix or open an issue otherwise.

xcaruso · 2023-04-06T20:25:29Z

Is it normal that github mentions that the file src/sage/combinat/crystals/key_crystals.py is deleted?

tornaria · 2023-04-06T20:34:41Z

Is it normal that github mentions that the file src/sage/combinat/crystals/key_crystals.py is deleted?

It is deleted in #35418. It shows up here because of the merge. That and the change to src/sage/schemes/elliptic_curve/cm.py come from that PR so you can ignore for review. I merged it here so the linter is happy and we know the current PR doesn't introduce lint failures.

The current PR only changes two lines of code, the rest is all adding a bunch of new tests that fail without this PR and pass with this PR.

xcaruso · 2023-04-06T21:05:29Z

I see, thanks for the answer.
Then, I approve your PR.

The precision for relaxed p-adics has 3 components: `(default_prec, halting_prec, secure)` where the last one `secure` is a boolean defaulting to `False`. However, the `construction()` method doesn't know about it: ``` sage: K = QpER(5, secure=True) sage: K.construction(forbid_frac_field=True) (Completion[5, prec=(20, 40)], Rational Field) sage: R = ZpER(5, secure=True) sage: R.construction() (Completion[5, prec=(20, 40)], Integer Ring) ``` This has two undesired consequences for the `change()` method: a. The `secure` attribute is not copied: ``` sage: K.is_secure() True sage: K.change().is_secure() False sage: R.is_secure() True sage: R.change().is_secure() False ``` b. The `check=False` option is broken: ``` sage: K.change(check=False) ... ValueError: not enough values to unpack (expected 3, got 2) sage: R.change(check=False) ... ValueError: not enough values to unpack (expected 3, got 2) ``` Fixing the `construction()` method solves both issues. After this commit: ``` sage: K = QpER(5, secure=True) sage: K.construction(forbid_frac_field=True) (Completion[5, prec=(20, 40, True)], Rational Field) sage: K.change().is_secure() True sage: K.change(check=False) 5-adic Field handled with relaxed arithmetics sage: K.change(check=False).is_secure() True sage: R = ZpER(5, secure=True) sage: R.construction() (Completion[5, prec=(20, 40, True)], Integer Ring) sage: R.change().is_secure() True sage: R.change(check=False) 5-adic Ring handled with relaxed arithmetics sage: R.change(check=False).is_secure() True ```

tornaria · 2023-04-06T23:37:12Z

Rebased to 10.0.beta8

github-actions · 2023-04-07T01:24:52Z

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

gh-35442: Make Qp.integer_ring() faster. ### 📚 Description The method pAdicGeneric.integer_ring() uses LocalGeneric.change() to turn a p-adic field into a p-adic ring. The latter calls a factory function which, by default, checks primality of p. However, when p came from a Qp this step is not necessary. We avoid it by adding `check=False` to the call to `LocalGeneric.change()` in `pAdicGeneric.integer_ring()`. This results in significant time savings for large primes, e.g. in the current test suite: Before this commit: ``` sage: R = Qp(next_prime(10^60)) sage: timeit('R.integer_ring()') 25 loops, best of 3: 22.2 ms per loop sage: %time TestSuite(R).run() CPU times: user 14.4 s, sys: 44 µs, total: 14.4 s Wall time: 14.4 s ``` After this commit: ``` sage: R = Qp(next_prime(10^60)) sage: timeit('R.integer_ring()') 625 loops, best of 3: 68 μs per loop sage: %time TestSuite(R).run() CPU times: user 714 ms, sys: 239 µs, total: 715 ms Wall time: 717 ms ``` Doctest of `padic_base_leaves.py` goes down from ~33 to ~5 seconds. Note that the `check=False` option for the `change()` method in relaxed type is broken, so this needs #35441. Other than that this is a one- liner. ### 📝 Checklist - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. ### ⌛ Dependencies - #35441 URL: #35442 Reported by: Gonzalo Tornaría Reviewer(s): roed314

tornaria requested a review from xcaruso April 5, 2023 16:42

tornaria added t: bug s: needs review c: padics labels Apr 5, 2023

tornaria requested a review from roed314 April 5, 2023 16:43

tornaria mentioned this pull request Apr 5, 2023

Make Qp.integer_ring() faster. #35442

Merged

2 tasks

xcaruso approved these changes Apr 6, 2023

View reviewed changes

tornaria mentioned this pull request Apr 6, 2023

Don't call sylvester_matrix with zero polynomials #35449

Merged

4 tasks

tornaria added 2 commits April 6, 2023 20:36

Run TestSuite for padic relaxed types with secure=True

1d0a04a

tornaria force-pushed the Qp_relaxed_construction branch from d9dbfe5 to 1d0a04a Compare April 6, 2023 23:36

roed314 approved these changes Apr 7, 2023

View reviewed changes

tornaria added s: positive review and removed s: needs review labels Apr 7, 2023

vbraun merged commit 34797a8 into sagemath:develop Apr 13, 2023

tornaria deleted the Qp_relaxed_construction branch March 1, 2025 14:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

Fix construction functor for p-adic relaxed types.#35441

Fix construction functor for p-adic relaxed types.#35441
vbraun merged 2 commits intosagemath:developfrom
tornaria:Qp_relaxed_construction

tornaria commented Apr 5, 2023

Uh oh!

roed314 commented Apr 5, 2023

Uh oh!

roed314 commented Apr 5, 2023

Uh oh!

mkoeppe commented Apr 6, 2023

Uh oh!

tornaria commented Apr 6, 2023

Uh oh!

tornaria commented Apr 6, 2023

Uh oh!

tornaria commented Apr 6, 2023

Uh oh!

xcaruso commented Apr 6, 2023

Uh oh!

tornaria commented Apr 6, 2023

Uh oh!

xcaruso commented Apr 6, 2023

Uh oh!

tornaria commented Apr 6, 2023

Uh oh!

github-actions bot commented Apr 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Comments

Conversation

tornaria commented Apr 5, 2023

📝 Checklist

Uh oh!

roed314 commented Apr 5, 2023

Uh oh!

roed314 commented Apr 5, 2023

Uh oh!

mkoeppe commented Apr 6, 2023

Uh oh!

tornaria commented Apr 6, 2023

Uh oh!

tornaria commented Apr 6, 2023

Uh oh!

tornaria commented Apr 6, 2023

Uh oh!

xcaruso commented Apr 6, 2023

Uh oh!

tornaria commented Apr 6, 2023

Uh oh!

xcaruso commented Apr 6, 2023

Uh oh!

tornaria commented Apr 6, 2023

Uh oh!

github-actions bot commented Apr 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants