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

Piecewise: added as_expr_set_pair property #2859

Merged
merged 1 commit into from Feb 18, 2014
Merged

Conversation

hargup
Copy link
Contributor

@hargup hargup commented Feb 1, 2014

This came up in the discussion at https://groups.google.com/forum/#!topic/sympy/x7IgsNoofNE.
This method will be useful in general while handling Piecewise functions. A few examples can be

  • in evaluating imageset for Piecewise functions
  • in plotting Piecewise function. Currently plot cannot plot even simple Piecewise functions like Piecewise((x, x > 0), (-x, True))
  • in solving expressions like Piecewise(((x - 2)**2, x >= 0), (0, True)). See line 305 sympy.functions.elementary.tests.test_piecewise

@smichr
Copy link
Member

smichr commented Feb 5, 2014

Can the name be as_expr_set? (Do we need the "_pair" part?)

@hargup
Copy link
Contributor Author

hargup commented Feb 6, 2014

Matthew is working on creating SetExpr at #2721 because expr_set sounds close to set_expr "_pair" part might help us to avoid the confusion in names.

@smichr
Copy link
Member

smichr commented Feb 7, 2014

Can anyone else give a +1 on this? It seems ok to me.

@@ -508,6 +508,16 @@ def __eval_cond(cls, cond):
return diff.is_zero
return None

@property
def as_expr_set_pair(cls, input_set=S.UniversalSet):
Copy link
Member

Choose a reason for hiding this comment

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

why do you use a parameter in a property function? Shouldn't you just set U = S.UniversalSet 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.

Sorry, this rose out of some confusion in my mind. I have removed it.

@mrocklin
Copy link
Member

This looks reasonable to me. I might add an s to the end of the name.

@@ -508,6 +508,16 @@ def __eval_cond(cls, cond):
return diff.is_zero
return None

@property
def as_expr_set_pair(cls):
Copy link
Contributor

Choose a reason for hiding this comment

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

why as_expr_set_pair is a property, but as_set isn't?

Copy link
Member

Choose a reason for hiding this comment

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

The first argument should be self, not cls. Only use cls on classmethod (meaning __new__ or anything with a @classmethod decorator).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why as_expr_set_pair is a property, but as_set isn't?

Yes, maybe as_set should also have been a property. Shall I correct it ?

@asmeurer
Copy link
Member

Yes please add an s

@hargup
Copy link
Contributor Author

hargup commented Feb 15, 2014

I have made the recommended changes.

@asmeurer
Copy link
Member

I don't think it should be a property. All the other as_* functions in SymPy are not properties.

@hargup
Copy link
Contributor Author

hargup commented Feb 15, 2014

I don't think it should be a property. All the other as_* functions in SymPy are not properties

I removed @property decorator from as_expr_set_pair. I think we should document this and other sympy specific coding conventions/patterns. See #2907 (comment).

@skirpichev
Copy link
Contributor

Will merge in 24hr

skirpichev added a commit that referenced this pull request Feb 18, 2014
Piecewise: added as_expr_set_pair property
@skirpichev skirpichev merged commit 96bad57 into sympy:master Feb 18, 2014
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.

None yet

5 participants