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

Fix pickling/deepcopying of floats when using gmpy2 backend #13530

Merged
merged 2 commits into from Nov 11, 2017

Conversation

richardotis
Copy link
Contributor

@richardotis richardotis commented Oct 24, 2017

Fixes gh-7457, closes gh-10674, and fixes gh-12895.

This is a rebased fix by @ovolve and a minimal test to address a bad interaction with pickling/deepcopying of Float objects when gmpy2 is installed (i.e., the "Argument is not an mpz" ValueError). When a future version of gmpy2 is released to address the underlying issue, this fix should be forward-compatible.

cc @asmeurer @casevh

@@ -19,6 +19,7 @@
SYMPY_INTS, int_info)
import mpmath
import mpmath.libmp as mlib
from mlib.backend import MPZ
Copy link
Member

Choose a reason for hiding this comment

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

This line is wrong.

@asmeurer
Copy link
Member

If this is based on a fix by @ovolve please keep the original commit metadata so that the authorship is correct.

@richardotis
Copy link
Contributor Author

I think technically the original author was @casevh in #7457 (comment)

@richardotis
Copy link
Contributor Author

Or more accurately in #7457 (comment)

@ylemkimon
Copy link
Member

You can cherry-pick commits from #10674 or (re)base the branch on it.

@richardotis
Copy link
Contributor Author

The patch from @ovolve is a two-line change that doesn't apply cleanly to current master. I don't understand how to preserve that commit metadata for this patch.

@richardotis
Copy link
Contributor Author

Alright, I think I figured it out. The commit history should be more consistent now.

@richardotis
Copy link
Contributor Author

@asmeurer This is ready for review

@asmeurer
Copy link
Member

You can also use the original commit and do a git merge to get it up to date with master.

@richardotis
Copy link
Contributor Author

Any blockers for this?

@@ -29,6 +30,7 @@
from .evaluate import global_evaluate

from sympy.utilities.exceptions import SymPyDeprecationWarning
from mpmath.libmp.backend import MPZ
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate import

def test_core_float_copy():
# See gh-7457
y = Symbol("x") + 1.0
copy.deepcopy(y) # does not raise TypeError ("argument is not an mpz")
Copy link
Member

Choose a reason for hiding this comment

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

Can you use check function like other tests?

@richardotis
Copy link
Contributor Author

Ready for another look

@ylemkimon
Copy link
Member

LGTM. Edited PR description to auto-close mentioned issues and PRs. Thank you for your contribution!

@ylemkimon ylemkimon merged commit d1b0989 into sympy:master Nov 11, 2017
@richardotis richardotis deleted the fix_gmpy2 branch November 11, 2017 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Argument is not an mpz TypeError when using both multiprocessing and gmpy
4 participants