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

Remove the cmp_to_key() helper function #5773

Open
vperic opened this Issue Sep 5, 2011 · 9 comments

Comments

Projects
None yet
3 participants
@vperic
Copy link
Contributor

vperic commented Sep 5, 2011

A helper function, cmp_to_key, was introduced with commit d68d33. The problem was that Python 3 no longer supports the cmp() function, and it cannot be used as an argument in sort() or sorted(). The idea behind the helper function was to avoid those issues for the moment and leave it for the future. As Python 3 support is (mostly) in now, the future is now. :)

cmp_to_key should not be used any more. This probably means changing our __cmp__ methods to rich comparisons but I'm not sure yet so lets keep this as a first goal. I will work on this. I do consider it low priority, though, as it doesn't really impact any functionality. 

See also what Aaron said in this comment[1]: "It appears that in a lot of cases, compare_pretty is being used with cmp_to_key.  This is wrong.  It should just use default_sort_key (or some variation of sort_key())."

Original issue for #5773: http://code.google.com/p/sympy/issues/detail?id=2674
Original author: https://code.google.com/u/108713607268198052411/
Original owner: https://code.google.com/u/108713607268198052411/

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Sep 4, 2011

**Blocking:** 4590  

Referenced issues: #4590
Original comment: http://code.google.com/p/sympy/issues/detail?id=2674#c1
Original author: https://code.google.com/u/asmeurer@gmail.com/

@vperic

This comment has been minimized.

Copy link
Contributor

vperic commented Sep 10, 2011

Well, this was embarrassingly simple to fix actually. As you said, most of the uses of Basic.compare_pretty can be replaced seamlessly with default_sort_key, and I've done just that in a branch. Unfortunately, there's a use in core/mul.py that cannot be just replaced with default_sort_key. Per a comment there, it's used just to assure a canonical ordering, but if I use default_sort_key, I get the following error (twice in two different core tests):

  File "/home/vperic/devel/sympy/sympy/core/tests/test_expr.py", line 183, in test_leadterm
    assert (1/x**2+1+x+x**2).leadterm(x)[1] == -2
  File "/home/vperic/devel/sympy/sympy/core/expr.py", line 1860, in leadterm
    raise ValueError("cannot compute leadterm(%s, %s), got c=%s" % (self, x, c))
ValueError: cannot compute leadterm(x**2 + x + 1 + x**(-2), x), got c=x**(-2)

As this is kinda deep down the core code I'm a little confused. Aaron, do you have any ideas? That's the last use of cmp_to_key. (I guess when I was first converting, this was what I tried to convert and gave up, instead preferring to add the cmp_to_key wrapper)

In any case, I've pushed my work to my github. It's on top of smichr's pull request 590 as those errors were annoying me. See it at: https://github.com/vperic/sympy/tree/cmp_to_key

**Status:** Started  

Original comment: http://code.google.com/p/sympy/issues/detail?id=2674#c2
Original author: https://code.google.com/u/108713607268198052411/

@vperic

This comment has been minimized.

Copy link
Contributor

vperic commented Sep 11, 2011

Aaron, I've applied your patch ( https://gist.github.com/1208773 ) but it unfortunately breaks other things, see the attached file for the log. I'll mess around with this a bit more. I applied your other patch and fixed the Python 3 issue, though.

**Cc:** asmeurer@gmail.com  

Original comment: http://code.google.com/p/sympy/issues/detail?id=2674#c3
Original author: https://code.google.com/u/108713607268198052411/

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Sep 11, 2011

Can you push this up to a branch?

Original comment: http://code.google.com/p/sympy/issues/detail?id=2674#c4
Original author: https://code.google.com/u/asmeurer@gmail.com/

@vperic

This comment has been minimized.

Copy link
Contributor

vperic commented Sep 11, 2011

Check the cmp_to_key branch. Expect rebases. Thanks for taking a look at this, I'm a bit lost.

Original comment: http://code.google.com/p/sympy/issues/detail?id=2674#c5
Original author: https://code.google.com/u/108713607268198052411/

@vperic

This comment has been minimized.

Copy link
Contributor

vperic commented Oct 9, 2011

Hmmm. Mateusz did some recent work related to this so I have to redo my branch from scratch (not yet ready). Still, looking at his patch aa94e72 (deprecate Basic.compare_pretty()) it occurs to me that we cannot remove cmp_to_key as long as we have to support old ordering -- I'm not sure we can guarantee the ordering is "old" if I change the function with whatever, and we can't have Python 3 support with the cmp_to_key function. So, my question is: Could we simply not define the old ordering for Python 3 and be done with it? The reasoning being, as the old ordering was already "old" by the time we got Python 3 support, then everyone on Python 3 should be using the "new" ordering anyway. Thoughts?

Of course, maybe I'm just missing some obvious fix for this? I could just replace the relevant line with default_sort_key, but obviously that's not "old" ordering anymore. Mateusz, what do you think?

**Cc:** matt...@gmail.com  

Original comment: http://code.google.com/p/sympy/issues/detail?id=2674#c6
Original author: https://code.google.com/u/108713607268198052411/

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Oct 10, 2011

That's a reasonable possibility. Otherwise, we just keep cmp_to_key around until we drop the old ordering.

To quote Mateusz on the other issue, "The old order will most certainly die naturally when Py3k port is ready, but I don't mind removing it earlier."

Original comment: http://code.google.com/p/sympy/issues/detail?id=2674#c7
Original author: https://code.google.com/u/asmeurer@gmail.com/

@vperic

This comment has been minimized.

Copy link
Contributor

vperic commented Oct 10, 2011

@vperic

This comment has been minimized.

Copy link
Contributor

vperic commented Oct 10, 2011

Alright, so based on the other issue, we'll drop old ordering two versions from now, so this is blocked on that. 

Or, again, I could just not define it in Python 3 if we consider removing cmp_to_key that important.

**Blockedon:** 1491  

Original comment: http://code.google.com/p/sympy/issues/detail?id=2674#c9
Original author: https://code.google.com/u/108713607268198052411/

@vperic vperic added imported labels Mar 7, 2014

@jcrist jcrist removed the Priority: Low label Apr 17, 2015

skirpichev added a commit to skirpichev/diofant that referenced this issue Nov 2, 2016

Add regression tests & mention closed issues
    close sympy/sympy#3112 (MrvAsympt was added in diofant#6)
    close sympy/sympy#9173 (test was added in 5a510ac)
    close sympy/sympy#9808 (fixed in 09e539b)
    close sympy/sympy#9341 (fixed in af98a00)
    close sympy/sympy#9908 (fixed in cc3fa8d)
    close sympy/sympy#6171 (test added in d278031)
    close sympy/sympy#9276 (diagnose_imports.py removed in ab8c535)
    close sympy/sympy#10201 (fixed in 0d0fc5f)
    close sympy/sympy#9057 (test was added in 8290a0c)
    close sympy/sympy#11159 (test was added in ffb76cb)
    close sympy/sympy#2839 (new AST transformers are used, see diofant#278 and diofant#167)
    close sympy/sympy#11081 (see ed01e16 and bb92329)
    close sympy/sympy#10974 (see 73fc425)
    close sympy/sympy#10806 (test in 539929a)
    close sympy/sympy#10801 (test in 2fe3da5)
    close sympy/sympy#9549 (test in 88bdefa)
    close sympy/sympy#4231 (test was added in fb411d5)
    close sympy/sympy#8634 (see 2fcbb58)
    close sympy/sympy#8481 (see 1ef20d3)
    close sympy/sympy#9956 (fixed in a34735f)
    close sympy/sympy#9747 (see e117c60)
    close sympy/sympy#7853 (see 3e4fbed)
    close sympy/sympy#9634 (see 2be03f5)
    close sympy/sympy#8500 (fixed in diofant#104 and finally in diofant#316)
    close sympy/sympy#9192 (see 9bf622f)
    close sympy/sympy#7130 (see e068fa3)
    close sympy/sympy#8514 (see b2d543b)
    close sympy/sympy#9334 (see 90de625)
    close sympy/sympy#8229 (see 9755b89)
    close sympy/sympy#8061 (see 7054f06)
    close sympy/sympy#7872 (tested in diofant#6)
    close sympy/sympy#3496 (tested in test_log_symbolic)
    close sympy/sympy#2929 (see da7db7a)
    close sympy/sympy#8203 (oo is not a real, see diofant#36)
    close sympy/sympy#7649 (0 is imaginary since diofant#8)
    close sympy/sympy#7256 (fixed in c0a4549)
    close sympy/sympy#6783 (see cb28d63)
    close sympy/sympy#5662 (is_integer issue fixed in 6bfa9f8, there is no is_bounded anymore)
    close sympy/sympy#5295 (fixed with diofant#354)
    close sympy/sympy#4856 (we now have flake/pep tests)
    close sympy/sympy#4555 (flake8 enabled after diofant#214)
    close sympy/sympy#5773 (cmp_to_key removed after diofant#164 and c9acbf0)
    close sympy/sympy#5484 (see above)

    Added regression tests:
    from https://groups.google.com/forum/#!topic/sympy/LkTMQKC_BOw
    fixes sympy/sympy#8825 (probably via diofant#209)
    fixes sympy/sympy#8635
    fixes sympy/sympy#8157
    fixes sympy/sympy#7872
    fixes sympy/sympy#7599
    fixes sympy/sympy#6179
    fixes sympy/sympy#5415
    fixes sympy/sympy#2865
    fixes sympy/sympy#5907
    fixes sympy/sympy#11722

    Closes diofant#347

skirpichev added a commit to skirpichev/diofant that referenced this issue Nov 2, 2016

Add regression tests & mention closed issues
    close sympy/sympy#3112 (MrvAsympt was added in diofant#6)
    close sympy/sympy#9173 (test was added in 5a510ac)
    close sympy/sympy#9808 (fixed in 09e539b)
    close sympy/sympy#9341 (fixed in af98a00)
    close sympy/sympy#9908 (fixed in cc3fa8d)
    close sympy/sympy#6171 (test added in d278031)
    close sympy/sympy#9276 (diagnose_imports.py removed in ab8c535)
    close sympy/sympy#10201 (fixed in 0d0fc5f)
    close sympy/sympy#9057 (test was added in 8290a0c)
    close sympy/sympy#11159 (test was added in ffb76cb)
    close sympy/sympy#2839 (new AST transformers are used, see diofant#278 and diofant#167)
    close sympy/sympy#11081 (see ed01e16 and bb92329)
    close sympy/sympy#10974 (see 73fc425)
    close sympy/sympy#10806 (test in 539929a)
    close sympy/sympy#10801 (test in 2fe3da5)
    close sympy/sympy#9549 (test in 88bdefa)
    close sympy/sympy#4231 (test was added in fb411d5)
    close sympy/sympy#8634 (see 2fcbb58)
    close sympy/sympy#8481 (see 1ef20d3)
    close sympy/sympy#9956 (fixed in a34735f)
    close sympy/sympy#9747 (see e117c60)
    close sympy/sympy#7853 (see 3e4fbed)
    close sympy/sympy#9634 (see 2be03f5)
    close sympy/sympy#8500 (fixed in diofant#104 and finally in diofant#316)
    close sympy/sympy#9192 (see 9bf622f)
    close sympy/sympy#7130 (see e068fa3)
    close sympy/sympy#8514 (see b2d543b)
    close sympy/sympy#9334 (see 90de625)
    close sympy/sympy#8229 (see 9755b89)
    close sympy/sympy#8061 (see 7054f06)
    close sympy/sympy#7872 (tested in diofant#6)
    close sympy/sympy#3496 (tested in test_log_symbolic)
    close sympy/sympy#2929 (see da7db7a)
    close sympy/sympy#8203 (oo is not a real, see diofant#36)
    close sympy/sympy#7649 (0 is imaginary since diofant#8)
    close sympy/sympy#7256 (fixed in c0a4549)
    close sympy/sympy#6783 (see cb28d63)
    close sympy/sympy#5662 (is_integer issue fixed in 6bfa9f8, there is no is_bounded anymore)
    close sympy/sympy#5295 (fixed with diofant#354)
    close sympy/sympy#4856 (we now have flake/pep tests)
    close sympy/sympy#4555 (flake8 enabled after diofant#214)
    close sympy/sympy#5773 (cmp_to_key removed after diofant#164 and c9acbf0)
    close sympy/sympy#5484 (see above)

    Added regression tests:
    from https://groups.google.com/forum/#!topic/sympy/LkTMQKC_BOw
    fixes sympy/sympy#8825 (probably via diofant#209)
    fixes sympy/sympy#8635
    fixes sympy/sympy#8157
    fixes sympy/sympy#7872
    fixes sympy/sympy#7599
    fixes sympy/sympy#6179
    fixes sympy/sympy#5415
    fixes sympy/sympy#2865
    fixes sympy/sympy#5907
    fixes sympy/sympy#11722

    Closes diofant#347
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment