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

Misc fixes #654

Merged
merged 5 commits into from Jun 24, 2018

Conversation

2 participants
@skirpichev
Copy link
Collaborator

skirpichev commented Jun 23, 2018

No description provided.

@skirpichev skirpichev added this to the 0.10 milestone Jun 23, 2018

@skirpichev skirpichev force-pushed the skirpichev:misc branch 3 times, most recently from bb949fd to 24a3ace Jun 23, 2018

@skirpichev skirpichev changed the title [wip] Misc fixes Misc fixes Jun 24, 2018

@skirpichev skirpichev force-pushed the skirpichev:misc branch from 5c627c8 to c5d27c9 Jun 24, 2018

@skirpichev skirpichev merged commit 7d63874 into diofant:master Jun 24, 2018

3 checks passed

codecov/patch 99% of diff hit (target 97%)
Details
codecov/project Absolute coverage decreased by -<1% but relative coverage increased by +2% compared to 62d97e7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@skirpichev skirpichev deleted the skirpichev:misc branch Jun 24, 2018

@skirpichev

This comment has been minimized.

Copy link
Collaborator

skirpichev commented Aug 1, 2018

@asmeurer, I don't think 4030e3d should go in sympy, but Rational.p/q properties looks too cryptic. Why not provide numerator/denominator properties? This would make Rational and Integer types more compatible with python numbers hierarchy. (In fact, Rational class probably will be fully compatible after this.)

@asmeurer

This comment has been minimized.

Copy link
Contributor

asmeurer commented Aug 1, 2018

I agree it should support the ABCs. It looks like there's an open issue for it sympy/sympy#12134. The p and q properties should be maintained for backwards compatibility. Either way, they are intended for internal use only, since they return ints, not Integer.

@skirpichev

This comment has been minimized.

Copy link
Collaborator

skirpichev commented Aug 1, 2018

Either way, they are intended for internal use only, since they return ints, not Integer.

@asmeurer, people can think that attribute is private if it name starts with underscore. That's not just a widely adopted convention, Python import system has some additional support for this.

But people can't guess that attribute is private, if its naming doesn't suggest that. And they will use this, for sure, e.g. see here.

sympy/sympy#12134

Easy to fix? Oh, I don't think so, given you have ">>" for Implies and so on.

@asmeurer

This comment has been minimized.

Copy link
Contributor

asmeurer commented Aug 1, 2018

>> isn't currently defined on Integer/Rational. Regardless, I don't see an issue with making the logic operators do bitwise operations on integers, since integers aren't booleans. This would actually give a strong reason to have a clear separation of the two.

skirpichev added a commit to skirpichev/Mathics that referenced this pull request Aug 19, 2018

Don't use p/q properties for Rational/Integer's
Short name indicate private stuff in fact, see
diofant/diofant#654

Unfortunately, SymPy numbers are not compatible
with Python's ABC hierarchy, there are no
numerator/denominator properties.

This fix uses public methods of SymPy and make
Mathics codebase more compatible with Diofant.

skirpichev added a commit to skirpichev/Mathics that referenced this pull request Oct 9, 2018

Don't use p/q properties for Rational/Integer's
Short name indicate private stuff in fact, see
diofant/diofant#654

Unfortunately, SymPy numbers are not compatible
with Python's ABC hierarchy, there are no
numerator/denominator properties.

This fix uses public methods of SymPy and make
Mathics codebase more compatible with Diofant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment