Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Add sorted_args and fix the sort keys for some unordered collections. #1446

Closed
wants to merge 5 commits into from

7 participants

@scolobb

This pull request adds sorted_args to Basic, attempts to make all the code which needs sorted args use this new property. Furthermore, this pull request makes FiniteSet and Dict produce hash-independent sort keys.

@scolobb

#1429 requires these changes (or something with a similar effect) to pass the tests.

@travisbot

This pull request fails (merged e2741fe1 into 147b462).

@Krastanov
Collaborator

SymPy Bot Summary: :red_circle: There were test failures.

@scolobb: Please fix the test failures.

Test command: setup.py test
master hash: 147b462
branch hash: e2741fe14428a18f3375013eeccd3e64e3653177

Interpreter 1: :red_circle: There were test failures.

Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYs9UiDA

Interpreter 2: :red_circle: There were test failures.

Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY6cUiDA

Interpreter 3: :red_circle: There were test failures.

Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYmN0iDA

Build HTML Docs: :red_circle: There were test failures.

Docs build command: make html-errors
Sphinx version: 1.1.3

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYt_wiDA

Automatic review by SymPy Bot.

@scolobb

Sorry, forgot to drop the .pyc's :-(

@travisbot

This pull request passes (merged 058dfb5f into 147b462).

@smichr
Collaborator

What do you think about adding a function sorted_args so any iterable can be thus sorted? Otherwise sorted(it, key=default_sort_key) must be written as Tuple(*it).sorted_args.

@smichr
Collaborator

Otherwise, this looks good to me. Let's see if there are other comments.

@asmeurer
Owner

SymPy Bot Summary: :red_circle: There were test failures.

@scolobb: Please fix the test failures.

Interpreter 1: :red_circle: There were test failures.

Interpreter: None (2.5.0-final-0)
Architecture: Darwin (32-bit)
Cache: yes
Test command: setup.py test
master hash: 9ad73da
branch hash: 058dfb5fc46588ef276dd60153dc08000132e89b

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYtNUiDA

Interpreter 2: :red_circle: There were test failures.

Interpreter: None (2.6.6-final-0)
Architecture: Darwin (32-bit)
Cache: yes
Test command: setup.py test
master hash: 9ad73da
branch hash: 058dfb5fc46588ef276dd60153dc08000132e89b

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY0fQiDA

Interpreter 3: :red_circle: There were test failures.

Interpreter: None (2.7.2-final-0)
Architecture: Darwin (32-bit)
Cache: yes
Test command: setup.py test
master hash: 9ad73da
branch hash: 058dfb5fc46588ef276dd60153dc08000132e89b

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY9JYiDA

Interpreter 4: :red_circle: There were test failures.

Interpreter: /sw/bin//python2.6 (2.6.7-final-0)
Architecture: Darwin (64-bit)
Cache: yes
Test command: setup.py test
master hash: 9ad73da
branch hash: 058dfb5fc46588ef276dd60153dc08000132e89b

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY6ewiDA

Interpreter 5: :red_circle: There were test failures.

Interpreter: /sw/bin//python2.7 (2.7.3-final-0)
Architecture: Darwin (64-bit)
Cache: yes
Test command: setup.py test
master hash: 9ad73da
branch hash: 058dfb5fc46588ef276dd60153dc08000132e89b

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY7e8hDA

Interpreter 6: :eight_spoked_asterisk: All tests have passed.

Interpreter: None (3.2.2-final-0)
Architecture: Darwin (32-bit)
Cache: yes
Test command: setup.py test
master hash: 9ad73da
branch hash: 058dfb5fc46588ef276dd60153dc08000132e89b

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYx6YiDA

Interpreter 7: :red_circle: There were test failures.

Interpreter: None (3.3.0-beta-1)
Architecture: Darwin (32-bit)
Cache: yes
Test command: setup.py test
master hash: 9ad73da
branch hash: 058dfb5fc46588ef276dd60153dc08000132e89b

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY9ZYiDA

Interpreter 8: :red_circle: There were test failures.

Interpreter: /sw/bin//python3.2 (3.2.3-final-0)
Architecture: Darwin (64-bit)
Cache: yes
Test command: setup.py test
master hash: 9ad73da
branch hash: 058dfb5fc46588ef276dd60153dc08000132e89b

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYlLYiDA

Interpreter 9: :red_circle: There were test failures.

Interpreter: None (3.3.0-beta-1)
Architecture: Darwin (64-bit)
Cache: yes
Test command: setup.py test
master hash: 9ad73da
branch hash: 058dfb5fc46588ef276dd60153dc08000132e89b

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYuPwiDA

Build HTML Docs: :eight_spoked_asterisk: All tests have passed.

Sphinx version: 1.1.2
Docs build command: make html-errors
master hash: 9ad73da
branch hash: 058dfb5fc46588ef276dd60153dc08000132e89b

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYyKYiDA

Automatic review by SymPy Bot.

@Krastanov
Collaborator

SymPy Bot Summary: :red_circle: There were test failures.

@scolobb: Please fix the test failures.

Test command: setup.py test
master hash: 9ad73da
branch hash: 058dfb5fc46588ef276dd60153dc08000132e89b

Interpreter 1: :eight_spoked_asterisk: All tests have passed.

Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYjI8iDA

Interpreter 2: :red_circle: There were test failures.

Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYyc0iDA

Interpreter 3: :red_circle: There were test failures.

Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYtdUiDA

Build HTML Docs: :eight_spoked_asterisk: All tests have passed.

Docs build command: make html-errors
Sphinx version: 1.1.3

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYmd0iDA

Automatic review by SymPy Bot.

@ness01

I'm very much +1 on this (modulo the minor comments I left on the commits).
@asmeurer @rlamy (and of course everyone else), what do you think?

@scolobb

@ness01, I have added the tests.

@smichr, could you please take a look at the sorted_args function I added in the latest commit? I am not sure I have understood your original idea properly, sorry.

@scolobb scolobb FiniteSet._complement: Don't use default_sort_key in sorting.
Some time ago I have made this method sort the elements by
default_sort_key.  This is redundant however, because _complement only
works for FiniteSet's containing numbers, which can be sorted directly,
without invoking default_sort_key.
ae77da1
@scolobb

I have also fixed a Union-related docstring in sets.py.

@travisbot

This pull request fails (merged 548782a1 into 9ad73da).

@travisbot

This pull request fails (merged d9b19c18 into 9ad73da).

@travisbot

This pull request passes (merged 0257dc2f into 9ad73da).

@smichr
Collaborator

re sorted_args: yep, that looks great.

@scolobb

re sorted_args: yep, that looks great.

Cool :-) Thanks for the feedback!

@Krastanov
Collaborator

SymPy Bot Summary: :red_circle: There were test failures.

@scolobb: Please fix the test failures.

Test command: setup.py test
master hash: 9ad73da
branch hash: 0257dc2f609e5d8d42bd3a25376a97b77a9510fe

Interpreter 1: :red_circle: There were test failures.

Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYwf8hDA

Interpreter 2: :red_circle: There were test failures.

Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY1fQiDA

Interpreter 3: :red_circle: There were test failures.

Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY6-wiDA

Build HTML Docs: :eight_spoked_asterisk: All tests have passed.

Docs build command: make html-errors
Sphinx version: 1.1.3

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY9pYiDA

Automatic review by SymPy Bot.

@scolobb scolobb referenced this pull request
Merged

Diagram Layout #1429

@rlamy
Collaborator

Er... Sorting the args of object of an object whose args already have a meaningful order (e.g. a Pow) doesn't make any sense. Also, adding sorted_args() makes code harder to read and creates more-than-one-way-to-do-it.

@asmeurer
Owner

I think it should be a private attribute. It could be misleading for users, who might think it means a mathematical ordering.

@scolobb

@asmeurer, I have done sorted_args -> _sorted_args; is that what you meant?

@rlamy, I thought you meant exactly this kind of implementation when you said in message on the mailing list that

That could be useful for many kinds of objects, not just FiniteSet

Further, do you think sorted_args() (the global function) should be removed? I'd rather wait for @smichr's feedback on this matter.

@smichr
Collaborator
@smichr
Collaborator

If we have the sorted_args function then we don't need the _sorted_args property, do we? (And Ronan has a point about this not being applicable to some objects.)

@travisbot

This pull request passes (merged 92fb0cbc into 9ad73da).

@scolobb

If we have the sorted_args function then we don't need the _sorted_args property, do we? (And Ronan has a point about this not being applicable to some objects.)

I'll wait for more feedback and will then change the pull request according to the opinion of the majority.

I am fairly supportive of this change, mainly because it will minimise the number of changes to Basic.

Although I respect the preferrably only-one-way principle, in this case I think that having a sorted_args is akin to having sorted() and .sort()

If we do remove sorted_args, the problem will go away altogether :-)

@Krastanov
Collaborator

SymPy Bot Summary: :red_circle: There were test failures.

@scolobb: Please fix the test failures.

Test command: setup.py test
master hash: 3519951
branch hash: 92fb0cbc4bc12fc8b2890638479d84a425c32c7a

Interpreter 1: :red_circle: There were test failures.

Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY68UiDA

Interpreter 2: :red_circle: There were test failures.

Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYvfwiDA

Interpreter 3: :red_circle: There were test failures.

Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY7uwiDA

Build HTML Docs: :red_circle: There were test failures.

Docs build command: make html-errors
Sphinx version: 1.1.3

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY_JYiDA

Automatic review by SymPy Bot.

@ness01

So what's the decision here?

@rlamy
Collaborator

For me, the point of .sorted_args is to return a canonical, platfom-independent, version of .args, so that obj.func(*obj.sorted_args) == obj == obj.func(*obj.args).

@scolobb

How about defining sorted_args to throw an error (not sure which, yet) by default in Basic, and redefining where necessary?

sympy/core/tests/test_sets.py
@@ -452,3 +452,14 @@ def test_universalset():
x = Symbol('x')
assert U.as_relational(x) == True
assert U.union(Interval(2,4)) == U
+
+def test_hash_independence():
+ x = Symbol('x')
+ s1 = FiniteSet(3, 2, x, 3)
+ assert s1.sort_key() == ((5, 0, 'FiniteSet'), (3, (((1, 0,
@asmeurer Owner

I don't know if we should test the exact value of the sort key.

@scolobb
scolobb added a note

OK, I'll drop this test then. I do get the point that the sort key is more of internal stuff and may change freely as long as it works as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@asmeurer
Owner

Yes, I meant that it should be ._sorted_args.

And the sorted_args() function seems redundant, especially given that the property is a private one.

@scolobb

Yes, I meant that it should be ._sorted_args.

Great, 92fb0cb does that.

And the sorted_args() function seems redundant, especially given that the property is a private one.

OK, I'll take care of that, then.

@smichr
Collaborator
@asmeurer
Owner

I think that would be better than throwing an error.

@smichr
Collaborator

Like free_symbols, which puts the other-than-default behavior on the object, the Basic version of _sorted_args could sort and if you don't like this (Pow, nc-Mul) you should override it. Pow and Mul need attention in this regard;

>>> m = (B*A)
>>> Mul(*m.sorted_args) == m
False
>>> p=x**2
>>> Pow(*p.sorted_args) == p
False
@asmeurer
Owner

It seems to be that it would be better for compatibility's sake if it worked the other way (non-sorted by default). Then at the very least if a class with unordered doesn't override it, the worst that will happen is that it won't be resilient against hash randomization related issues.

@rlamy
Collaborator

I agree with Aaron.

@smichr
Collaborator

Is there any reason not to rename default_sort_key to just sort_key?

@asmeurer
Owner

The name sort_key is already taken, and means something slightly different. I believe the difference has to do with the order keyword.

@smichr
Collaborator
@scolobb

Cool, thanks for the feedback! I will have _sorted_args return args in Basic and will override this behaviour in FiniteSet and Dict.

It's only taken as a method:

This, again, sounds like .sort() vs. sorted() to me. I don't really have a strong opinion, but .sort() and sorted() do have slightly different names.

@smichr
Collaborator

This, again, sounds like .sort() vs. sorted() to me. I don't really have a strong opinion, but .sort() and sorted() do have slightly different names.

sort and sorted are verbs; sort_key is a noun; default_sort_key is just a wrapper for sort_key (when dealing with a Basic object) and supplies a key that can be used for non-Basic objects. Other wrappers have identical names as the method (e.g. expand(), simplify(), diff(), etc...).

@scolobb scolobb Basic: Add _sorted_args and use them in sort_key.
In the derived classes which don't fix a certain ordering for their
.args, _sorted_args should returns the list of arguments sorted with
default_sort_key.  By default, _sorted_args returns the same thing as
args.
748785d
sympy/core/basic.py
@@ -296,6 +296,26 @@ def class_key(cls):
"""Nice order of classes. """
return 5, 0, cls.__name__
+ def _sort_key(self, sorted=False):
+ """
+ Returns a sort key based on ``self.args`` if ``sorted`` is
+ ``False``, and based on ``self._sorted_args`` otherwise.
+ """
+ if sorted:
+ args = self._sorted_args
+ else:
+ args = self.args
+
+ # XXX: remove this when issue #2070 is fixed
+ def inner_key(arg):
+ if isinstance(arg, Basic):
+ return arg.sort_key()
@smichr Collaborator
smichr added a note

The order keyword should be passed to _sort_key and used here (arg.sort_key(order=order)).

@scolobb
scolobb added a note

Oh, great! I was wondering what that keyword argument was for.

Note that after having _sorted_args return args by default, it doesn't make sense to have this additional _sort_key function. I have modified sorted_key to always use _sorted_args. I think this code has become more elegant after I have followed the suggestions in the comments.

@smichr Collaborator
smichr added a note

When you do that, keys will be order-sensitive:

>>> (1+x).sort_key()==(1+x).sort_key('rev-lex')
False
@scolobb
scolobb added a note

The order argument was originally (I mean, currently in master) unused in Basic. I wonder whether that's a typo.

When you do that, keys will be order-sensitive:

Oh, I see. Thanks for explanation!

@smichr Collaborator
smichr added a note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
scolobb added some commits
@scolobb scolobb Basic.sort_key: Use the order keyword argument.
This argument was not used before.  It's meaning is to assure
order-dependent sort keys.  The example by Christopher Smith says:

>>> (1+x).sort_key()==(1+x).sort_key('rev-lex')
False
2815e97
@scolobb scolobb FiniteSet, Dict: Override _sorted_args to sorted the args.
This is done in order to assure that the sort keys of these two classes
are hash-independent.
887fd51
@scolobb scolobb sets.set_sort_fn: Fix the docstring.
The docstring of this function was not properly updated when the code
has been changed.  This commit fixes that.
e1a8872
@scolobb

I have ninja-edited this pull request to only contain the changes over which everyone seemed to agree. @asmeurer, @rlamy, @smichr, @ness01, could you please take a look at it once again?

I believe this pull request only introduces a minimal number of changes now, and it suffices to fix the problem with FiniteSet and Dict which gave me trouble in #1429.

I am not sure though as to which other classes should redefine _sorted_args and in which way. I know Mul and Add can handle noncommutative terms, but I haven't really investigated the matter. Perhaps, we could leave those classes untouched and, should the sort key issue arise for one of them, just fix it by overloading _sorted_args? I could open a corresponding issue, for example.

@ness01

Looks good to me.

@smichr smichr commented on the diff
sympy/core/basic.py
((6 lines not shown))
else:
return arg
- args = len(self.args), tuple([ inner_key(arg) for arg in self.args ])
+ args = self._sorted_args
@smichr Collaborator
smichr added a note

I would leave this as self.args otherwise you have to go to the method to find out what is gives you -- in this case, unsorted args (and I think that's confusing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@travisbot

This pull request passes (merged e1a8872 into a67d688).

@smichr smichr commented on the diff
sympy/core/sets.py
@@ -667,9 +667,8 @@ def as_relational(self, symbol):
def set_sort_fn(s):
@smichr Collaborator
smichr added a note

it seems like this should be called set_sort_key

@smichr Collaborator
smichr added a note

actaully, shouldn't this be defined in Set as sort_key? and then the function can be deleted and the places where it is used would use default-sort_key instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@smichr smichr commented on the diff
sympy/core/sets.py
@@ -667,9 +667,8 @@ def as_relational(self, symbol):
def set_sort_fn(s):
"""
- Sort by infimum if possible
-
- Otherwise sort by hash. Try to put these at the end.
+ Sort by infimum if possible. Otherwise sort by
+ ``default_sort_key``.
@smichr Collaborator
smichr added a note

Return sort key of infimum (if possible) otherwise the value of default_sort_key of the set.

(The phrasing now makes it sound like something is sorted.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@smichr smichr commented on the diff
sympy/core/sets.py
@@ -1219,7 +1218,7 @@ def _complement(self):
raise ValueError("%s: Complement not defined for symbolic inputs"
%self)
- args = sorted(self.args, key=default_sort_key)
+ args = sorted(self.args)
@smichr Collaborator
smichr added a note

are args always going to sort from -oo to oo without the default_sort_key? And if so, then the -sorted_args routine below doesn't need to use default-sort_key and this line here at 1222 can just be args = self._sorted_args.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@smichr
Collaborator

ok, that's all of my comments.

@Krastanov
Collaborator

SymPy Bot Summary: :red_circle: There were test failures.

@scolobb: Please fix the test failures.

Test command: setup.py test
master hash: a67d688
branch hash: e1a8872

Interpreter 1: :red_circle: There were test failures.

Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYwvwiDA

Interpreter 2: :red_circle: There were test failures.

Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY9uwiDA

Interpreter 3: :eight_spoked_asterisk: All tests have passed.

Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYh4wjDA

Build HTML Docs: :red_circle: There were test failures.

Docs build command: make html-errors
Sphinx version: 1.1.3

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY9u8hDA

Automatic review by SymPy Bot.

@scolobb

I'm closing this pull request since the changes here have become a part of #1458.

@scolobb scolobb closed this
@smichr
Collaborator

I have a sorted-args branch that adds the properties to Add, Mul, and Pow in case it's ever needed. Pretty simple, though, so it won't be bad if someone forgets about them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 29, 2012
  1. @scolobb

    FiniteSet._complement: Don't use default_sort_key in sorting.

    scolobb authored
    Some time ago I have made this method sort the elements by
    default_sort_key.  This is redundant however, because _complement only
    works for FiniteSet's containing numbers, which can be sorted directly,
    without invoking default_sort_key.
Commits on Aug 1, 2012
  1. @scolobb

    Basic: Add _sorted_args and use them in sort_key.

    scolobb authored
    In the derived classes which don't fix a certain ordering for their
    .args, _sorted_args should returns the list of arguments sorted with
    default_sort_key.  By default, _sorted_args returns the same thing as
    args.
  2. @scolobb

    Basic.sort_key: Use the order keyword argument.

    scolobb authored
    This argument was not used before.  It's meaning is to assure
    order-dependent sort keys.  The example by Christopher Smith says:
    
    >>> (1+x).sort_key()==(1+x).sort_key('rev-lex')
    False
  3. @scolobb

    FiniteSet, Dict: Override _sorted_args to sorted the args.

    scolobb authored
    This is done in order to assure that the sort keys of these two classes
    are hash-independent.
  4. @scolobb

    sets.set_sort_fn: Fix the docstring.

    scolobb authored
    The docstring of this function was not properly updated when the code
    has been changed.  This commit fixes that.
This page is out of date. Refresh to see the latest.
View
14 sympy/core/basic.py
@@ -320,11 +320,12 @@ def sort_key(self, order=None):
# XXX: remove this when issue #2070 is fixed
def inner_key(arg):
if isinstance(arg, Basic):
- return arg.sort_key()
+ return arg.sort_key(order)
else:
return arg
- args = len(self.args), tuple([ inner_key(arg) for arg in self.args ])
+ args = self._sorted_args
@smichr Collaborator
smichr added a note

I would leave this as self.args otherwise you have to go to the method to find out what is gives you -- in this case, unsorted args (and I think that's confusing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ args = len(args), tuple([ inner_key(arg) for arg in args ])
return self.class_key(), args, S.One.sort_key(), S.One
def __eq__(self, other):
@@ -631,6 +632,15 @@ def args(self):
"""
return self._args
+ @property
+ def _sorted_args(self):
+ """
+ The same as ``args``. Derived classes which don't fix an
+ order on their arguments should override this method to
+ produce the sorted representation.
+ """
+ return self.args
+
def iter_basic_args(self):
"""
Iterates arguments of ``self``.
View
5 sympy/core/containers.py
@@ -204,3 +204,8 @@ def __contains__(self, key):
def __lt__(self, other):
return self.args < other.args
+
+ @property
+ def _sorted_args(self):
+ from sympy.utilities import default_sort_key
+ return sorted(self.args, key=default_sort_key)
View
12 sympy/core/sets.py
@@ -667,9 +667,8 @@ def as_relational(self, symbol):
def set_sort_fn(s):
@smichr Collaborator
smichr added a note

it seems like this should be called set_sort_key

@smichr Collaborator
smichr added a note

actaully, shouldn't this be defined in Set as sort_key? and then the function can be deleted and the places where it is used would use default-sort_key instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
"""
- Sort by infimum if possible
-
- Otherwise sort by hash. Try to put these at the end.
+ Sort by infimum if possible. Otherwise sort by
+ ``default_sort_key``.
@smichr Collaborator
smichr added a note

Return sort key of infimum (if possible) otherwise the value of default_sort_key of the set.

(The phrasing now makes it sound like something is sorted.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
"""
try:
val = s.inf
@@ -1219,7 +1218,7 @@ def _complement(self):
raise ValueError("%s: Complement not defined for symbolic inputs"
%self)
- args = sorted(self.args, key=default_sort_key)
+ args = sorted(self.args)
@smichr Collaborator
smichr added a note

are args always going to sort from -oo to oo without the default_sort_key? And if so, then the -sorted_args routine below doesn't need to use default-sort_key and this line here at 1222 can just be args = self._sorted_args.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
intervals = [] # Build up a list of intervals between the elements
intervals += [Interval(S.NegativeInfinity, args[0], True, True)]
@@ -1265,3 +1264,8 @@ def _eval_evalf(self, prec):
def _hashable_content(self):
return (self._elements,)
+
+ @property
+ def _sorted_args(self):
+ from sympy.utilities import default_sort_key
+ return sorted(self.args, key=default_sort_key)
View
3  sympy/core/tests/test_basic.py
@@ -124,3 +124,6 @@ def test_preorder_traversal():
expr = z + w*(x+y)
assert list(preorder_traversal([expr], key=default_sort_key)) == \
[[w*(x + y) + z], w*(x + y) + z, z, w*(x + y), w, x + y, x, y]
+
+def test_sorted_args():
+ assert b21._sorted_args == b21.args
Something went wrong with that request. Please try again.