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.

Sign up for GitHub

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

Jump to bottom

Fix serialization breakage due to sympy.Rational now being a numbers.Number #2655

Merged
merged 6 commits into from
Dec 17, 2019
Merged

Fix serialization breakage due to sympy.Rational now being a numbers.Number #2655

merged 6 commits into from
Dec 17, 2019

Conversation

Strilanc
Copy link
Contributor

@Strilanc Strilanc commented Dec 16, 2019 •
edited

Fixes #2650
Fixes #2646

@Strilanc
Fix serialization breakage due to sympy.Rational now being a numbers.…
38055cb 
…Number

- Reorder the sympy checks to come before generic number checks
- Rename json.py to json_serialization.py to avoid collisions with the built-in json library
@googlebot googlebot added the cla: yes Makes googlebot stop complaining. label Dec 16, 2019
@Strilanc Strilanc mentioned this pull request Dec 16, 2019
Closed
mpharrigan
mpharrigan approved these changes Dec 16, 2019
View reviewed changes
Copy link
Collaborator

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

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

Awesome. Happy to see it was such a simple fix. I imagine sympy changed something so isinstance(obj, numbers.Integral) now returns True for sympy.Integer?

@Strilanc Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Dec 16, 2019
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Dec 16, 2019
@CirqBot
Copy link
Collaborator

CirqBot commented Dec 16, 2019

Automerge cancelled: Need a fresh 🍪.

@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Dec 16, 2019
@Strilanc Strilanc added automerge Tells CirqBot to sync and merge this PR. (If it's running.) and removed front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Dec 16, 2019
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Dec 16, 2019
@KevinVillela
Copy link
Contributor

Yayyyy thanks!

@Strilanc
Copy link
Contributor Author

I imagine sympy changed something so isinstance(obj, numbers.Integral) now returns True for sympy.Integer?

Yeah something like that.

@CirqBot
Copy link
Collaborator

CirqBot commented Dec 16, 2019

Automerge cancelled: A status check is failing.

@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Dec 16, 2019
@Strilanc
Copy link
Contributor Author

The OSX build is failing for some unknown reason

image

I have disabled it in this PR and will open an issue to re-enable it.

@Strilanc Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Dec 16, 2019
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Dec 16, 2019
@Strilanc Strilanc mentioned this pull request Dec 16, 2019
Closed
@CirqBot
Copy link
Collaborator

CirqBot commented Dec 16, 2019

Automerge cancelled: A status check is failing.

@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Dec 16, 2019
@Strilanc
Copy link
Contributor Author

You are killing me here, cirqbot.

@Strilanc
Copy link
Contributor Author

Urgh, just found sympy/sympy#18056 which puts a big blocker on using the new version of sympy. I am going to pin to the old one.

@Strilanc Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Dec 17, 2019
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Dec 17, 2019
@CirqBot CirqBot merged commit 9a5a09e into master Dec 17, 2019
@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Dec 17, 2019
@CirqBot CirqBot deleted the jsonfix branch December 17, 2019 00:26
@CirqBot CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Dec 17, 2019
@mpharrigan mpharrigan mentioned this pull request Dec 17, 2019
Closed
This was referenced Jan 30, 2020
Merged
Merged
CirqBot pushed a commit that referenced this pull request Jan 31, 2020
@drewrisinger
requirements: remove sympy pin (#2721)
db9a9ed 
Sympy v1.5.1 is now available, fixes sympy/sympy#18056.

See https://github.com/sympy/sympy/releases/tag/sympy-1.5.1 & changelog: https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5.1

Ref: #2648 #2655
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpharrigan mpharrigan mpharrigan approved these changes

@vtomole vtomole Awaiting requested review from vtomole

Assignees
No one assigned
Labels
cla: yes Makes googlebot stop complaining.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cirq does not work with latest version of Sympy (1.5) Cirq tests appear to be failing
5 participants
@Strilanc @CirqBot @KevinVillela @mpharrigan @googlebot