Opened 6 months ago

Closed 3 months ago

#26792 closed enhancement (fixed)

py3: Stop to use sort on KeyConvertingDict object

Reported by: vklein Owned by:
Priority: major Milestone: sage-8.7
Component: python3 Keywords:
Cc: Merged in:
Authors: Vincent Klein Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: c9c1047 (Commits) Commit: c9c10475a2dae3691bcfb330c99522df82e387b2
Dependencies: Stopgaps:

Description

With python3 we currently have 40 failures of type

TypeError: '<' not supported between instances of 'KeyConvertingDict' and 'KeyConvertingDict'

These errors comes from the call of V.sort() in multi_polynomial_ideal.py", line 2381, variety function.

The root reason for calling sort is for doctest displaying :

commit 17f42824d8941b23825fea1a6d82278bd5ef6c74 Author: Carl Witty <cwitty@…> Date: Wed Oct 24 18:36:37 2007 -0700

Sort results of variety(), for better cross-architecture doctest compatibility. This compensates for the fact that the results of Singular's triangular decomposition algorithm triangLfak appear in a different order on different architectures. (It would be better to fix this in triangular_decomposition(), but the obvious approach of sorting its result failed due to TRAC #991.)

As it will not make a lot of sense to implement comparison operators for KeyConvertingDict? this ticket aim at solving the sorting problems at doctests level rather than at python source code level.

Change History (34)

comment:1 Changed 6 months ago by vklein

  • Reviewers set to Vincent Klein

comment:2 Changed 6 months ago by vklein

  • Authors set to Vincent Klein
  • Reviewers Vincent Klein deleted

comment:3 Changed 6 months ago by vklein

  • Branch set to u/vklein/26792

comment:4 Changed 6 months ago by git

  • Commit set to 4b8a3e59a962bb6c003162ff0ab8fe986c111d06

Branch pushed to git repo; I updated commit sha1. New commits:

4b8a3e5Trac #26781: replace dict.iteritems calls by dict.items

comment:5 Changed 6 months ago by vklein

  • Dependencies set to #23572

Add #23572 as dependency for mpoly renaming.

comment:6 Changed 5 months ago by chapoton

  • Milestone changed from sage-8.5 to sage-8.7

comment:7 Changed 4 months ago by git

  • Commit changed from 4b8a3e59a962bb6c003162ff0ab8fe986c111d06 to b78d1c42b3d3dbab29075404285d965ded990ee6

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

b32170freviewer suggestion about independent set test
324b483Merge branch 'public/french_book_python3' in 8.6.b0
a00c64clivre sage doctests: fix floating errors
3c6139aMerge branch 'public/french_book_python3' in 8.6.b1
6d6fb70partial fix for numerical noise on osX
17d97d8Merge branch 'public/french_book_python3' in 8.6
6da3629book doctests : adding some tolerance again
5fd7867Trac #26792: py3: Stop to use sort on KeyConvertingDict object
140623fTrac #26792: py3: fix test.french_book_mpoly module
b78d1c4Trac #26792: replace dict.iteritems calls by dict.items

comment:8 Changed 4 months ago by git

  • Commit changed from b78d1c42b3d3dbab29075404285d965ded990ee6 to ceee97f1c993abec0f194e23631ab799f86e71db

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

822e4deTrac #26792: py3: Stop to use sort on KeyConvertingDict object
ceee97fTrac #26792: replace dict.iteritems calls by dict.items

comment:9 Changed 4 months ago by vklein

  • Dependencies #23572 deleted

Ticket rebased on 8.7.beta1.

comment:10 Changed 4 months ago by git

  • Commit changed from ceee97f1c993abec0f194e23631ab799f86e71db to 3c0660cb0219f4d4f9a5f111050ddf652dc2d6ac

Branch pushed to git repo; I updated commit sha1. New commits:

3c0660cTrac #26792: fix mpoly.py doctests

comment:11 Changed 4 months ago by vklein

Fix mpoly.py doctests and remove #23572 dependency.

comment:12 Changed 4 months ago by git

  • Commit changed from 3c0660cb0219f4d4f9a5f111050ddf652dc2d6ac to 9f3984b12fba4c0ffec6ff761a5faf76637fad3b

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

9f3984bTrac #26792: fix mpoly.py doctests

comment:13 Changed 4 months ago by git

  • Commit changed from 9f3984b12fba4c0ffec6ff761a5faf76637fad3b to f5ce2fa06f5f4c7da222461a8a3477d0302dbe75

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f5ce2faTrac #26792: fix mpoly.py doctests

comment:14 Changed 4 months ago by vklein

  • Dependencies set to #23572

ticket on hold in order to let priority for #23572

comment:15 Changed 3 months ago by chapoton

time to start again with this ticket ; this should be a great step forward when done

comment:16 Changed 3 months ago by chapoton

  • Branch changed from u/vklein/26792 to public/ticket/26792
  • Commit changed from f5ce2fa06f5f4c7da222461a8a3477d0302dbe75 to 2f908f0961293e36c42bd1e2fe81153fafd44b0b
  • Dependencies #23572 deleted
  • Status changed from new to needs_review

New commits:

2f908f0Merge branch 'u/vklein/26792' in 8.7.b4

comment:17 Changed 3 months ago by chapoton

  • Status changed from needs_review to needs_work

remains one failing doctest

comment:18 Changed 3 months ago by git

  • Commit changed from 2f908f0961293e36c42bd1e2fe81153fafd44b0b to f4a2bfa9f1490a4795ddf3d5232262d991325ae8

Branch pushed to git repo; I updated commit sha1. New commits:

f4a2bfatrac 26792 fixing one doctest

comment:19 Changed 3 months ago by chapoton

  • Status changed from needs_work to needs_review

comment:20 Changed 3 months ago by jhpalmieri

I get a merge conflict with 8.7.beta4: src/sage/tests/books/computational-mathematics-with-sagemath/mpoly_doctest.py

comment:21 Changed 3 months ago by jhpalmieri

There is a related doctest failure in misc/converting_dict.py. Four Python 3 failures in the develop branch are replaced by 1 with this branch.

comment:22 Changed 3 months ago by git

  • Commit changed from f4a2bfa9f1490a4795ddf3d5232262d991325ae8 to 945857ffe6b2f66ecbdeb84369ad0d402e51c5be

Branch pushed to git repo; I updated commit sha1. New commits:

945857ftrac 26792 trying to fix a doctest

comment:23 Changed 3 months ago by jhpalmieri

The new test passes with py3 but not py2.

comment:24 follow-up: Changed 3 months ago by chapoton

  • Status changed from needs_review to needs_work

and before my last commit it was the contrary. Probably one should make sure that the repr of these "dicts" is conveniently sorted in the same way for both py2 and py3.

comment:25 Changed 3 months ago by vklein

I will fix that.

comment:26 Changed 3 months ago by git

  • Commit changed from 945857ffe6b2f66ecbdeb84369ad0d402e51c5be to 59c980e59a8492da4e8088809dcbb2f102d38e23

Branch pushed to git repo; I updated commit sha1. New commits:

59c980eTrac #26792: Fix doctest in converting_dict.py

comment:27 in reply to: ↑ 24 Changed 3 months ago by vklein

Replying to chapoton:

and before my last commit it was the contrary. Probably one should make sure that the repr of these "dicts" is conveniently sorted in the same way for both py2 and py3.

The __repr__ called by KeyConvertingDict is the one inherited from python's dict i don't think we should override it just for doctests compatibility. I prefer to modify the doctests.

comment:28 follow-up: Changed 3 months ago by chapoton

I was talking about the ipython representation. This is different (already for dict) from using str or repr. Using that in the repr would be better.

comment:29 Changed 3 months ago by git

  • Commit changed from 59c980e59a8492da4e8088809dcbb2f102d38e23 to 4dc152ad3be45d97b17669eccc9f943854c25368

Branch pushed to git repo; I updated commit sha1. New commits:

4dc152aTrac #26792: Fix a doctest in multi_polynomial_ideal.py

comment:30 in reply to: ↑ 28 Changed 3 months ago by vklein

Replying to chapoton:

I was talking about the ipython representation. This is different (already for dict) from using str or repr. Using that in the repr would be better.

I think you mean using Ipython pretty print. I don't know why it is like it's not called by default for KeyConvertingDict as it is for dict:

Adding some traces :

diff --git a/src/sage/misc/converting_dict.py b/src/sage/misc/converting_dict.py
index f9bdb17..f08e13f 100644
--- a/src/sage/misc/converting_dict.py
+++ b/src/sage/misc/converting_dict.py
@@ -140,6 +140,13 @@ class KeyConvertingDict(dict):
         key = self.key_conversion_function(key)
         return super(KeyConvertingDict, self).__setitem__(key, value)
 
+    def __repr__(self):
+        print("call __repr__")
+        return super(KeyConvertingDict, self).__repr__()
+
+    def _repr_pretty_(self, p, cycle):
+        print("pretty")
+
     def __delitem__(self, key):
         r"""
         Remove a mapping from the dictionary.

You get this:

sage: K.<x,y> = QQ[]
sage: I = ideal([x^2+2*y-5,x+y+3])
sage: v = I.variety(AA)[0]; v
call __repr__
{x: 4.464101615137755?, y: -7.464101615137755?}
sage: from IPython.lib.pretty import pprint
sage: pprint(v)
pretty
Last edited 3 months ago by vklein (previous) (diff)

comment:31 Changed 3 months ago by git

  • Commit changed from 4dc152ad3be45d97b17669eccc9f943854c25368 to c9c10475a2dae3691bcfb330c99522df82e387b2

Branch pushed to git repo; I updated commit sha1. New commits:

c9c1047Trac #26792: py3 fix doctests failing because ...

comment:32 Changed 3 months ago by vklein

  • Status changed from needs_work to needs_review

I fixed the remaining cases using #py2 #py3 flags as it seems to be the most readable solution right now.

There is still one failling test sort related to KeyConvertingDict:

File "src/sage/tests/books/computational-mathematics-with-sagemath/mpoly_doctest.py", line 409, in sage.tests.books.computational-mathematics-with-sagemath.mpoly_doctest
Failed example:
    J.variety(RealField(51))
Expected:
    [{y: 396340.890166545, x: -14.1660266425312}]
Got:
    [{y: 396340.890166545, x: -35.0231212211684}]

The difference being the value of x i think this is out the scope of this ticket.

comment:33 Changed 3 months ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, let us move on here.

comment:34 Changed 3 months ago by vbraun

  • Branch changed from public/ticket/26792 to c9c10475a2dae3691bcfb330c99522df82e387b2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.