Float: Fix use of MPZ constructor when unpickling #21996

mkoeppe · 2021-09-01T00:55:13Z

References to other Issues or PRs

Fixes #19690

Brief description of what is fixed or changed

Use MPZ constructor as documented.

Other comments

Release Notes

core
- A bug that preventing Float from being unpickled in SageMath was fixed.

sympy-bot · 2021-09-01T00:55:14Z

✅

Hi, I am the SymPy bot (v161). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

core
- A bug that preventing Float from being unpickled in SageMath was fixed. (#21996 by @mkoeppe)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.9.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->
Fixes #19690

#### Brief description of what is fixed or changed
Use `MPZ` constructor as documented.


#### Other comments


#### Release Notes

<!-- BEGIN RELEASE NOTES -->
* core
   * A bug that preventing `Float` from being unpickled in SageMath was fixed.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

oscarbenjamin · 2021-09-01T01:05:49Z

I forgot how monstrous Float.__new__ is.

Is it even necessary or useful to use MPZ there?

mkoeppe · 2021-09-01T01:17:21Z

I cannot answer this question; first time I look at this code.

mkoeppe · 2021-09-01T01:28:38Z

Failing tests:

_________________ sympy/core/tests/test_numbers.py:test_Float __________________
Traceback (most recent call last):
  File "/home/runner/work/sympy/sympy/sympy/core/tests/test_numbers.py", line 446, in test_Float
    x_str =  Float((0, '13333333333333', -52, 53))
  File "/home/runner/work/sympy/sympy/sympy/core/numbers.py", line 1160, in __new__
    assert num[1].startswith('0x')
AssertionError
________________________________________________________________________________
____________ sympy/core/tests/test_numbers.py:test_Float_from_tuple ____________
Traceback (most recent call last):
  File "/home/runner/work/sympy/sympy/sympy/core/tests/test_numbers.py", line 651, in test_Float_from_tuple
    a = Float((0, '1L', 0, 1))
  File "/home/runner/work/sympy/sympy/sympy/core/numbers.py", line 1160, in __new__
    assert num[1].startswith('0x')
AssertionError

These failing tests are wrong

oscarbenjamin · 2021-09-01T01:38:41Z

These failing tests are wrong

Why are they wrong? Should they not use strings?

mkoeppe · 2021-09-01T02:13:14Z

OK, it's actually a bit more complicated than I thought...
The problem is coming in from https://github.com/fredrik-johansson/mpmath/blob/c11db84b3237bd8fc6721f5a0c5d7c0c98a24dc1/mpmath/libmp/libmpf.py#L30 to_pickable, which creates these 4-tuples. @fredrik-johansson

If using the normal backend, it uses hex(...)[2:], i.e., stripping off the 0x prefix.
If using the Sage backends, it just uses hex, so the 0x is kept. Perhaps some earlier version of Sage did not include the 0x prefix? (So also from_pickable uses the MPZ constructor wrong in this case.)

mkoeppe · 2021-09-01T02:23:01Z

Indeed, https://trac.sagemath.org/ticket/26756 (merged in Sage 8.2) removed the Integer.__hex__ method; since then, calling hex on a Sage integer produces the prefix 0x.

mkoeppe · 2021-09-01T02:31:10Z

OK, here is a version that works with both types of inputs.

asmeurer · 2021-09-01T03:01:11Z

This looks better. We shouldn't break existing ways of constructing Floats, especially if there are tests for it.

mkoeppe · 2021-09-01T03:18:15Z

Should I add tests for the version with 0x?

…rmat

github-actions · 2021-09-01T03:43:06Z

Benchmark results from GitHub Actions

Lower numbers are good, higher numbers are bad. A ratio less than 1
means a speed up and greater than 1 means a slowdown. Green lines
beginning with + are slowdowns (the PR is slower then master or
master is slower than the previous release). Red lines beginning
with - are speedups.

Significantly changed benchmark results (PR vs master)

Significantly changed benchmark results (master vs previous release)

       before           after         ratio
     [ed9a550f]       [5c2dfe72]
     <sympy-1.8^0>                 
-      1.29±0.02s          172±4ms     0.13  dsolve.TimeDsolve01.time_dsolve
-      9.86±0.03s          5.11±0s     0.52  integrate.TimeIntegrationRisch02.time_doit(100)
-      9.74±0.02s       5.04±0.05s     0.52  integrate.TimeIntegrationRisch02.time_doit_risch(100)
-        83.9±4μs         35.6±2μs     0.42  matrices.TimeDiagonalEigenvals.time_eigenvals
-        98.0±3μs         64.3±4μs     0.66  matrices.TimeMatrixGetItem.time_ImmutableDenseMatrix_getitem
-         107±2μs       63.4±0.6μs     0.60  matrices.TimeMatrixGetItem.time_MutableSparseMatrix_getitem
-        98.3±4μs         62.3±4μs     0.63  solve.TimeMatrixArithmetic.time_dense_add(10, 0)
+         102±3μs          160±7μs     1.57  solve.TimeMatrixArithmetic.time_dense_add(10, 5)
+      14.0±0.4μs         22.2±1μs     1.58  solve.TimeMatrixArithmetic.time_dense_add(3, 0)
+      16.7±0.4μs         37.6±2μs     2.25  solve.TimeMatrixArithmetic.time_dense_add(3, 5)
+      23.6±0.6μs         47.7±2μs     2.02  solve.TimeMatrixArithmetic.time_dense_add(4, 5)
+        44.6±1μs       78.1±0.4μs     1.75  solve.TimeMatrixArithmetic.time_dense_add(6, 5)
-     1.49±0.05ms         289±30μs     0.19  solve.TimeMatrixArithmetic.time_dense_multiply(10, 0)
-        53.9±1μs       26.8±0.9μs     0.50  solve.TimeMatrixArithmetic.time_dense_multiply(3, 0)
+        89.3±3μs          146±7μs     1.64  solve.TimeMatrixArithmetic.time_dense_multiply(3, 5)
-         114±3μs         38.3±1μs     0.34  solve.TimeMatrixArithmetic.time_dense_multiply(4, 0)
-        318±10μs         74.6±3μs     0.23  solve.TimeMatrixArithmetic.time_dense_multiply(6, 0)
-         108±3μs         51.5±3μs     0.48  solve.TimeMatrixOperations.time_det(3, 0)
-         213±9μs          123±3μs     0.58  solve.TimeMatrixOperations.time_det(3, 2)
-         192±5μs         114±10μs     0.59  solve.TimeMatrixOperations.time_det(3, 5)
-         102±3μs         49.4±3μs     0.48  solve.TimeMatrixOperations.time_det_bareiss(3, 0)
-         199±3μs          124±4μs     0.62  solve.TimeMatrixOperations.time_det_bareiss(3, 2)
-         197±4μs          108±2μs     0.55  solve.TimeMatrixOperations.time_det_bareiss(3, 5)
-        106±10μs         47.9±1μs     0.45  solve.TimeMatrixOperations.time_det_berkowitz(3, 0)
-        211±10μs          121±3μs     0.57  solve.TimeMatrixOperations.time_det_berkowitz(3, 2)
-         189±4μs          111±5μs     0.59  solve.TimeMatrixOperations.time_det_berkowitz(3, 5)
+        893±30μs      1.42±0.08ms     1.59  solve.TimeMatrixOperations.time_det_berkowitz(4, 0)
+     1.11±0.06ms       2.16±0.2ms     1.94  solve.TimeMatrixOperations.time_det_berkowitz(4, 5)
+         345±9μs         562±20μs     1.63  solve.TimeMatrixOperations.time_rank(3, 0)
+        540±40μs         847±20μs     1.57  solve.TimeMatrixOperations.time_rank(4, 0)
+         143±6μs         218±10μs     1.53  solve.TimeMatrixOperations.time_rref(3, 0)
-      5.53±0.3ms       3.42±0.2ms     0.62  solve.TimeRationalSystem.time_linsolve(10)

Full benchmark results can be found as artifacts in GitHub Actions
(click on checks at the top of the PR).

mkoeppe · 2021-09-01T19:06:56Z

Ready for review

oscarbenjamin · 2021-09-01T20:15:59Z

Some tests failed although they seem unrelated:

________________________________________________________________________________
 sympy/matrices/expressions/tests/test_blockmatrix.py:test_BlockMatrix_2x2_inverse_numeric 
Traceback (most recent call last):
  File "/home/runner/work/sympy/sympy/sympy/matrices/expressions/blockmatrix.py", line 313, in schur
    inv = (d[mat].T*d[mat]).inv()*d[mat].T if generalized else d[mat].inv()
  File "/home/runner/work/sympy/sympy/sympy/matrices/matrices.py", line 2223, in inv
    return _inv(self, method=method, iszerofunc=iszerofunc,
  File "/home/runner/work/sympy/sympy/sympy/matrices/inverse.py", line 459, in _inv
    rv = M.inverse_GE(iszerofunc=iszerofunc)
  File "/home/runner/work/sympy/sympy/sympy/matrices/matrices.py", line 2208, in inverse_GE
    return _inv_GE(self, iszerofunc=iszerofunc)
  File "/home/runner/work/sympy/sympy/sympy/matrices/inverse.py", line 245, in _inv_GE
    raise NonInvertibleMatrixError("Matrix det == 0; not invertible.")
sympy.matrices.common.NonInvertibleMatrixError: Matrix det == 0; not invertible.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/runner/work/sympy/sympy/sympy/matrices/expressions/tests/test_blockmatrix.py", line 234, in test_BlockMatrix_2x2_inverse_numeric
    assert block_collapse(K.inv()).as_explicit() == K.as_explicit().inv()
  File "/home/runner/work/sympy/sympy/sympy/matrices/expressions/blockmatrix.py", line 722, in block_collapse
    result = rule(expr)
  File "/home/runner/work/sympy/sympy/sympy/strategies/core.py", line 8, in exhaustive_rl
    new, old = rule(expr), expr
  File "/home/runner/work/sympy/sympy/sympy/strategies/core.py", line 41, in chain_rl
    expr = rule(expr)
  File "/home/runner/work/sympy/sympy/sympy/strategies/core.py", line 8, in exhaustive_rl
    new, old = rule(expr), expr
  File "/home/runner/work/sympy/sympy/sympy/strategies/core.py", line 30, in conditioned_rl
    return rule(expr)
  File "/home/runner/work/sympy/sympy/sympy/strategies/core.py", line 92, in switch_rl
    return rl(expr)
  File "/home/runner/work/sympy/sympy/sympy/matrices/expressions/blockmatrix.py", line 821, in bc_inverse
    return blockinverse_2x2(Inverse(reblock_2x2(expr.arg)))
  File "/home/runner/work/sympy/sympy/sympy/matrices/expressions/blockmatrix.py", line 838, in blockinverse_2x2
    MI = expr.arg.schur(formula).I
  File "/home/runner/work/sympy/sympy/sympy/matrices/expressions/blockmatrix.py", line 325, in schur
    raise NonInvertibleMatrixError('The given matrix is not invertible. Please set generalized=True \
sympy.matrices.common.NonInvertibleMatrixError: The given matrix is not invertible. Please set generalized=True             to compute the generalized Schur Complement which uses Moore-Penrose Inverse

________________________________________________________________________________
_____ sympy/assumptions/tests/test_matrices.py:test_invertible_BlockMatrix _____
Traceback (most recent call last):
  File "/home/runner/work/sympy/sympy/sympy/assumptions/tests/test_matrices.py", line 54, in test_invertible_BlockMatrix
    assert ask(Q.invertible(BlockMatrix([
AssertionError

Closing and reopening to rerun the tests.

oscarbenjamin · 2021-09-01T21:16:54Z

Looks good. Thanks!

mkoeppe · 2021-09-01T21:24:03Z

BTW, the Sage doctest failures for FiniteSet should be gone now because I have pushed a change to https://trac.sagemath.org/ticket/32420

oscarbenjamin · 2021-09-01T21:28:19Z

BTW, the Sage doctests failures for FiniteSet should be gone now because I have pushed a change to https://trac.sagemath.org/ticket/32420

Thanks. I'm seeing some new failures like:

File "src/sage/symbolic/integration/external.py", line 247, in sage.symbolic.integration.external.giac_integrator
Failed example:
    giac_integrator(sqrt(x + sqrt(x)), x)
Expected:
    1/12*(2*sqrt(x)*(4*sqrt(x) + 1) - 3)*sqrt(x + sqrt(x))
    - 1/8*log(abs(2*sqrt(x + sqrt(x)) - 2*sqrt(x) - 1))
Got:
    1/12*(2*sqrt(x)*(4*sqrt(x) + 1) - 3)*sqrt(x + sqrt(x)) - 1/8*log(-2*sqrt(x + sqrt(x)) + 2*sqrt(x) + 1)

Is that perhaps related to this recent Maxima thread?
https://sourceforge.net/p/maxima/mailman/message/37343844/

mkoeppe · 2021-09-01T21:33:38Z

Thanks, this is https://trac.sagemath.org/ticket/29966#comment:14 and unrelated to sympy

oscarbenjamin · 2021-09-01T22:27:31Z

@mkoeppe how should we manage this part of the CI going forward?

I guess there are going to be continuously failing tests as various projects update. Should we forward reports of these or are they automatically recorded somewhere for sage?

If we can see failures related to something changed in sympy that would also need a change in sage then I guess it makes sense to open a sage ticket explaining the change. The example above is one where it seemed clear to me that sympy was not relevant but in general we won't always know if that's the case.

mkoeppe · 2021-09-01T23:21:43Z

@mkoeppe how should we manage this part of the CI going forward?

That's a great and important question.

I guess there are going to be continuously failing tests as various projects update. Should we forward reports of these or are they automatically recorded somewhere for sage?
If we can see failures related to something changed in sympy that would also need a change in sage then I guess it makes sense to open a sage ticket explaining the change.

Reporting as Sage tickets (after a cursory check that there is not already a ticket for it) is always welcome. We do have our automatic test runs but they do require some manual inspection, which is not happening for every beta and every platform that we support.

The example above is one where it seemed clear to me that sympy was not relevant but in general we won't always know if that's the case.

Currently, ci-sage.yml is set up to run against the merge of current SageMath develop branch (= latest beta version, which is updated every 1–2 weeks) and the branch of ticket https://trac.sagemath.org/ticket/32420 . After that ticket is merged into develop, this will be the same as testing against current SageMath develop.

So after merging https://trac.sagemath.org/ticket/32420 (probably after the SymPy 1.9 release), the baseline to compare with is https://github.com/sagemath/sage/actions/workflows/tox.yml, which runs automatically on each new beta pushed to develop.

oscarbenjamin · 2021-09-01T23:45:10Z

That sounds good. We'll keep an eye on the sage workflow and see how it goes.

the baseline to compare with is https://github.com/sagemath/sage/actions/workflows/tox.yml

There's a lot of red in that CI... I'm glad we now have proper integration tests for Sage. As it happens I have just today managed to make the rest of SymPy's CI both complete and green but the master commits will still show as red for now because of the sage workflow - I think that's the way it should be though because that is what CI is for.

mkoeppe · 2021-09-01T23:51:19Z

the baseline to compare with is https://github.com/sagemath/sage/actions/workflows/tox.yml

There's a lot of red in that CI...

Yes, exactly – that's what I meant with "manual inspection" being necessary. It's very complex and we can't just count green checkmarks, unfortunately. But from your side you'll be able to focus just on the two platforms that we put into ci-sage.yml: debian-buster-standard and archlinux-latest-standard.

mkoeppe mentioned this pull request Sep 1, 2021

copy.deepcopy(3.7*meter) raises an Exception #19690

Closed

Float: Fix use of MPZ constructor when unpickling

9737261

mkoeppe force-pushed the fix_use_of_MPZ_constructor branch from ef19d5c to 9737261 Compare September 1, 2021 02:30

sympy/core/tests/test_numbers.py: Add test for another Float tuple fo…

316900c

…rmat

oscarbenjamin closed this Sep 1, 2021

oscarbenjamin reopened this Sep 1, 2021

oscarbenjamin enabled auto-merge September 1, 2021 21:16

oscarbenjamin merged commit f6ebb44 into sympy:master Sep 1, 2021

oscarbenjamin mentioned this pull request Sep 1, 2021

State of the docs for release #22000

Closed

oscarbenjamin mentioned this pull request Sep 2, 2021

failure in master? #21968

Closed

asmeurer mentioned this pull request Jan 29, 2023

Incompatibilities in Sage mpmath cause downstream issues in SymPy sagemath/sage#25445

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Float: Fix use of MPZ constructor when unpickling #21996

Float: Fix use of MPZ constructor when unpickling #21996

mkoeppe commented Sep 1, 2021 •

edited by oscarbenjamin

Loading

sympy-bot commented Sep 1, 2021 •

edited

Loading

oscarbenjamin commented Sep 1, 2021

mkoeppe commented Sep 1, 2021

mkoeppe commented Sep 1, 2021

oscarbenjamin commented Sep 1, 2021

mkoeppe commented Sep 1, 2021

mkoeppe commented Sep 1, 2021

mkoeppe commented Sep 1, 2021

asmeurer commented Sep 1, 2021

mkoeppe commented Sep 1, 2021

github-actions bot commented Sep 1, 2021 •

edited

Loading

mkoeppe commented Sep 1, 2021

oscarbenjamin commented Sep 1, 2021

oscarbenjamin commented Sep 1, 2021

mkoeppe commented Sep 1, 2021 •

edited

Loading

oscarbenjamin commented Sep 1, 2021

mkoeppe commented Sep 1, 2021

oscarbenjamin commented Sep 1, 2021

mkoeppe commented Sep 1, 2021

oscarbenjamin commented Sep 1, 2021

mkoeppe commented Sep 1, 2021 •

edited

Loading

Float: Fix use of MPZ constructor when unpickling #21996

Float: Fix use of MPZ constructor when unpickling #21996

Conversation

mkoeppe commented Sep 1, 2021 • edited by oscarbenjamin Loading

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

sympy-bot commented Sep 1, 2021 • edited Loading

oscarbenjamin commented Sep 1, 2021

mkoeppe commented Sep 1, 2021

mkoeppe commented Sep 1, 2021

oscarbenjamin commented Sep 1, 2021

mkoeppe commented Sep 1, 2021

mkoeppe commented Sep 1, 2021

mkoeppe commented Sep 1, 2021

asmeurer commented Sep 1, 2021

mkoeppe commented Sep 1, 2021

github-actions bot commented Sep 1, 2021 • edited Loading

mkoeppe commented Sep 1, 2021

oscarbenjamin commented Sep 1, 2021

oscarbenjamin commented Sep 1, 2021

mkoeppe commented Sep 1, 2021 • edited Loading

oscarbenjamin commented Sep 1, 2021

mkoeppe commented Sep 1, 2021

oscarbenjamin commented Sep 1, 2021

mkoeppe commented Sep 1, 2021

oscarbenjamin commented Sep 1, 2021

mkoeppe commented Sep 1, 2021 • edited Loading

mkoeppe commented Sep 1, 2021 •

edited by oscarbenjamin

Loading

sympy-bot commented Sep 1, 2021 •

edited

Loading

github-actions bot commented Sep 1, 2021 •

edited

Loading

mkoeppe commented Sep 1, 2021 •

edited

Loading

mkoeppe commented Sep 1, 2021 •

edited

Loading