Set.sort_key, default_sort_key edits #1458
Conversation
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.
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.
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
This is done in order to assure that the sort keys of these two classes are hash-independent.
The docstring of this function was not properly updated when the code has been changed. This commit fixes that.
|
This is ready to go but someone else has to give the final ok of the |
|
Yeah, that's a problem with Stefan's bot. Let me run a more comprehensive (and hopefully bug-free) bot test. |
|
As to it being OK to merge, perhaps @scolobb should sign-off since it's based off his PR and he's hopefully pretty adept in these kinds of changes by now. |
|
|
||
| This key is supplied by the sort_key routine of Basic objects when | ||
| ``item`` is a Basic object or a string that sympifies to a Basic | ||
| object. Otherwise, this function supplies a values: |
There was a problem hiding this comment.
"this function supplies a tuple containing the following values" ?
|
@smichr , thank you for taking this up! I'm perfectly OK with the commits you have added, modulo the nitpicks in the comments. Sorry it took me so long to come around to this pull request. |
|
The Python 3 failures in my bot run are my fault. |
|
@asmeurer, there are some gremlins printing in http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYrrYiDA see the ask.py and ipython lines. Also, how do we keep track of the failures that are occuring? Perhaps I should open an issue? There is a matrixlib-related error and a plot error showing in http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYgvAhDA |
|
btw, @Krastanov (or others) I used tilde underlining so as not to mess up the docs (docscrape?) I'm hoping it will let them pass and not parse that as a header. I'll make the changes suggested and then commit this (after hearring about whether the tildes are a problem). |
|
Yeah open issues for those, except for the cython (autowrap) one, which already has an issue. |
|
For what it's worth, this looks good to me, too. |
|
I'm slowly getting the hang of Sphinx's errors. I think this will be the final edit of the docstring and then (if it passes) I will commit it. Any idea why scolobb doesn't show up in the github list of branches? |
|
This is a long standing github bug. It's because they only show the top 100 people. The work around is to enter the URL manually (the format is easy enough to figure out from the default). |
|
SymPy Bot Summary: 🔴 There were test failures. @smichr: Please fix the test failures. Test command: setup.py test Interpreter 1: ✳️ All tests have passed. Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY5ZsjDA Interpreter 2: ✳️ All tests have passed. Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY5P8hDA Interpreter 3: ✳️ All tests have passed. Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY6fQiDA Build HTML Docs: 🔴 There were test failures. Docs build command: make html-errors Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY_5MjDA Automatic review by SymPy Bot. |
The docstring explains why using the order hint may not
give the results one expects if using the default_sort_key
for sorting rather than canonical ordering.
The way around this would be
to create a new sort function that collects a set Expr items,
then assigns an order to each one as might be given if those
items were in an Add or Mul. The only catch is that 0 cannot appear in
an Add and 1 cannot appear in a Mul:
>>> Add(1/x, x ,1, 2, evaluate=0).as_ordered_terms('rev-lex')
[1/x, 2, 1, x]
>>> Add(1/x, x ,1, 2, evaluate=0).as_ordered_terms()
[x, 1, 2, 1/x]
>>> Mul(0, 1/x, x + 1, x**2, evaluate=0).as_ordered_factors()
[0, 1/x, x**2, x + 1]
Set.sort_key, default_sort_key edits
|
@smichr , thanks for the corrections and for getting this in! I don't remember seeing any issues related to the problem this pull request fixes; so I guess, nothing to close. |
This is a replacement for @scolobb's branch #1446 (he doesn't show up in the branch list in github so I can't send it directly to him).