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

Deprecate set.is_real property, use set.is_subset(Reals) instead #7996

Merged
merged 1 commit into from Sep 16, 2014
Merged

Deprecate set.is_real property, use set.is_subset(Reals) instead #7996

merged 1 commit into from Sep 16, 2014

Conversation

skirpichev
Copy link
Contributor

fixes #6212

(on top of #7861, see also #7972)

@skirpichev skirpichev added the sets label Sep 8, 2014
@smichr
Copy link
Member

smichr commented Sep 8, 2014

+1 to this commit

@skirpichev
Copy link
Contributor Author

But please note, that we can't merge this, unless we merge first #7861 or #7972.

I really like quantum stuff in the sympy tree, but this shouldn't block development for month!

@smichr
Copy link
Member

smichr commented Sep 8, 2014

Let's rebase on #7972 and commit.

@skirpichev skirpichev added the Needs Decision This needs discussion to resolve an undecided issue before further work can be done. label Sep 8, 2014
@skirpichev
Copy link
Contributor Author

Let's rebase on #7972 and commit.

Why on this? Yes, less "broken" tests here, but I did here a lot of changes in the quantum module.

@smichr
Copy link
Member

smichr commented Sep 8, 2014

You made changes in the quantum module by adding assumptions to symbols which are consistent with the existing test results (which suggests that this is what the quantum folks meant).

@skirpichev
Copy link
Contributor Author

On Mon, Sep 08, 2014 at 10:50:54AM -0700, Christopher Smith wrote:

You made changes in the quantum module by adding assumptions to symbols
which are consistent with the existing test results

Yes. And I did very questionable change in the represent.py for this.

(In my view, the usage of Symbol's is totally broken in the quantum
module and I don't see an easy way to fix this.)

(which suggests that this is what the quantum folks meant).

Or that they have very bad tests or(and) bad test coverage.

>>> represent(X*x*y)
x*DiracDelta(x - x_3)*DiracDelta(x_1 - y)
-x*DiracDelta(x - x_3)*DiracDelta(x_1 - y)*Heaviside(-x - oo) + x*DiracDelta(x - x_3)*DiracDelta(x_1 - y)
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way to make XKet and Xbra be real and bounded? Otherwise this result could be Piecewise((x_DiracDelta(x - x_3)_DiracDelta(x_1 - y), Ne(x, -oo)), (S.NaN, True)).

Copy link
Member

Choose a reason for hiding this comment

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

ditto below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any way to make XKet and Xbra be real and bounded?

I don't know what you mean by this. But if you do care about this test - we can preserve it, see #7972

But lets discuss #7972 and #7861 not in this pr.

@skirpichev
Copy link
Contributor Author

rebased

@skirpichev skirpichev removed the Needs Decision This needs discussion to resolve an undecided issue before further work can be done. label Sep 10, 2014
@skirpichev
Copy link
Contributor Author

it seems others are ok with this deprecation in #6212

we can merge this pr after either #7972 or #7861

@skirpichev
Copy link
Contributor Author

@asmeurer, can we consider this for 0.7.6?

@asmeurer
Copy link
Member

Absolutely. Is it ready to go?

@skirpichev
Copy link
Contributor Author

Absolutely. Is it ready to go?

The only problem - it depends on either #7861 or #7972.

@asmeurer
Copy link
Member

Can you summarize the differences? It says the only difference is the fixes in the quantum module. The one uses real=True, bounded=True, and I can't tell what the other does.

@asmeurer
Copy link
Member

Oh I guess the other one changes the test output.

@asmeurer
Copy link
Member

I actually have issues with both of those. They both introduce this strange new Heaviside(-x - oo) term, leading to

In [1]: integrate(-x*DiracDelta(x - y), (x, -oo, oo))
Out[1]: yHeaviside(-y - ∞) - y

If y is an infinity that is a nan and otherwise it is -y (the result you get in master for this integral).

@skirpichev
Copy link
Contributor Author

On Sat, Sep 13, 2014 at 02:32:07PM -0700, Aaron Meurer wrote:

I actually have issues with both of those. They both introduce this
strange new Heaviside(-x - oo) term, leading to

In [1]: integrate(-x*DiracDelta(x - y), (x, -oo, oo))
Out[1]: y⋅Heaviside(-y - ∞) - y

If y is an infinity that is a nan

Yes. That's why we can't simplify this term. -y can be oo.

@skirpichev
Copy link
Contributor Author

rebase on top of #7861

@skirpichev skirpichev added this to the 0.7.6 milestone Sep 14, 2014
@skirpichev
Copy link
Contributor Author

will merge in 24hr

skirpichev added a commit that referenced this pull request Sep 16, 2014
Deprecate set.is_real property, use set.is_subset(Reals) instead
@skirpichev skirpichev merged commit 1512c1d into sympy:master Sep 16, 2014
@skirpichev skirpichev deleted the remove-is_real-for-sets branch September 16, 2014 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Name clash: Basic.is_real vs Set.is_real
3 participants