mysterious doctest failure on dyck_word.py

[videlec]

Several times and on different computers make ptestlong produces this single error

sage -t --long --warn-long 81.8 src/sage/combinat/dyck_word.py 
********************************************************************** 
File "src/sage/combinat/dyck_word.py", line 3501, in 
sage.combinat.dyck_word.DyckWords_size.__init__ 
Failed example: 
      TestSuite(DyckWords(4,2)).run() 
Expected nothing 
Got: 
      ... 
      tester.assert_(isinstance(card, Integer)) 
      ... 
      ------------------------------------------------------------ 
      The following tests failed: _test_enumerated_set_iter_cardinality 
********************************************************************** 
1 item had failures: 

see: this thread on sage-devel

CC: @behackl @nthiery

Component: documentation

Author: Vincent Delecroix

Branch/Commit: 59cb8d2

Reviewer: Benjamin Hackl

Issue created by migration from https://trac.sagemath.org/ticket/18244

[videlec]

New commits:

75f0a4a

Trac 18244: be more verbose in _test_enumerated_set_iter_cardinality

[videlec]

Commit: 75f0a4a

[videlec]

Branch: public/18244

[behackl]

comment:2

And here is what can be found, based on the more verbose output:

./sage -bt src/sage/combinat/dyck_word.py gives

sage -t --warn-long 78.6 src/sage/combinat/dyck_word.py
**********************************************************************
File "src/sage/combinat/dyck_word.py", line 3501, in sage.combinat.dyck_word.DyckWords_size.__init__
Failed example:
    TestSuite(DyckWords(4,2)).run()
Expected nothing
Got:
    Failure in _test_enumerated_set_iter_cardinality:
    Traceback (most recent call last):
      File "/home/behackl/Programming/sage-6.7.beta1/local/lib/python2.7/site-packages/sage/misc/sage_unittest.py", line 282, in run
        test_method(tester = tester)
      File "/home/behackl/Programming/sage-6.7.beta1/local/lib/python2.7/site-packages/sage/categories/finite_enumerated_sets.py", line 463, in _test_enumerated_set_iter_cardinality
        "expected a Sage Integer and got {} of type {}".format(card,type(card)))
      File "/home/behackl/Programming/sage-6.7.beta1/local/lib/python/unittest/case.py", line 422, in assertTrue
        raise self.failureException(msg)
    AssertionError: expected a Sage Integer and got 9 of type <type 'int'>
    ------------------------------------------------------------
    The following tests failed: _test_enumerated_set_iter_cardinality
**********************************************************************

On the other hand, just testing without building (./sage -t src/sage/combinat/dyck_word.py) yields

sage -t --warn-long 78.6 src/sage/combinat/dyck_word.py
    [574 tests, 0.95 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------

It looks like a Python integer kills us directly after building sage, but not if we are just testing.

[videlec]

comment:3

Yes indeed. Very strange. Note that in a console

sage: DyckWords(4r, 2r).cardinality()
9
sage: type(_)
<type 'int'>

which is the expected behavior after #17852.

[videlec]

comment:4

Got it... it is because of unique representation! There is a cache of computed values of DyckWords(4,2). Hence

sage: D1 = DyckWords(4r, 2r)  # init with Python int
sage: D2 = DyckWords(4, 2)    # init with Sage integer
sage: D2.cardinality()
9
sage: type(_)
<type 'int'>

So if the block at lines 3583-3585

sage: all(all(DyckWords(p, q).cardinality()
....:           == len(DyckWords(p, q).list()) for q in range(p + 1))
....:      for p in range(7))

is ran before the TestSuite(DyckWords(4,2)).run() and the weak cache is still present we got the failure!

[behackl]

comment:5

Nice work, caching is definitely the problem! :-)

However, replacing range with srange at lines 3583-3585 does not seem to solve the problem for me quite yet; building and testing still yields an error. But I guess now its only a matter of finding the right range. ;-)

[sagetrac-git]

Changed commit from 75f0a4a to 5dfb836

[sagetrac-git]

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

5dfb836

Trac 18244: fix (indirectly) cardinality in dyck_word

[videlec]

comment:7

Replying to @behackl:

However, replacing range with srange at lines 3583-3585 does not seem to solve the problem for me quite yet; building and testing still yields an error. But I guess now its only a matter of finding the right range. ;-)

Argh. I was sure this was the one! I do not want to go through the file again.

[behackl]

comment:8

Some more information: the problem definitely is, that cardinality may return a Python integer. For testing purposes, I converted the output to a sage Integer, and as a result building and testing works.

However, even if I delete the example at lines 3583-3585, the error remains. I haven't really figured out yet where the Python integers magically appear.

[behackl]

comment:9

The magical appearance of the Python integers is still a mystery, but as your last commit kills all of those, it resolves the bug.

As soon as you set the ticket to needs_review, I'd be happy to finish this. :-)

[videlec]

Author: Vincent Delecroix

[simon-king-jena]

Changed author from Vincent Delecroix to none

[simon-king-jena]

comment:11

Replying to @behackl:

Some more information: the problem definitely is, that cardinality may return a Python integer. For testing purposes, I converted the output to a sage Integer, and as a result building and testing works.

However, even if I delete the example at lines 3583-3585, the error remains. I haven't really figured out yet where the Python integers magically appear.

I somehow disagree with that analysis that it is a problem that cardinality returns a Python integer. Yes, the category test suite verifies that cardinality returns a Sage integer. However (and that's why I Cc Nicolas) I don't think that the test should be so restrictive.

Hence, would it not be a better solution to modify the test so that it accepts both sage and python integers (and infinity)?

[videlec]

comment:12

Replying to @simon-king-jena:

Replying to @behackl:

Some more information: the problem definitely is, that cardinality may return a Python integer. For testing purposes, I converted the output to a sage Integer, and as a result building and testing works.

However, even if I delete the example at lines 3583-3585, the error remains. I haven't really figured out yet where the Python integers magically appear.

I somehow disagree with that analysis that it is a problem that cardinality returns a Python integer. Yes, the category test suite verifies that cardinality returns a Sage integer. However (and that's why I Cc Nicolas) I don't think that the test should be so restrictive.

I like this to be restrictive. See also #18159. I often write things like

    return S1.cardinality().factorial()

which would break if it was a Python integer. Your suggestion implies that I need to do

    from sage.rings.integer_ring import ZZ
    return ZZ(S1.cardinality()).factorial()

Moreover, in that particular case, I dislike the behavior of the example in comment:4. Depending on the execution you got a Python int or a Sage integer! This is very bad.

Hence, would it not be a better solution to modify the test so that it accepts both sage and python integers (and infinity)?

This has to be discussed (maybe on #18159). And which infinity?

Vincent

[simon-king-jena]

comment:13

That said, changing the code of Dyck words so that Sage integers are used is a good idea. Nonetheless I think that isinstance(card, Integer) is a bad idea. Instead, it should test card in NN.

[simon-king-jena]

comment:14

Replying to @videlec:

I like this to be restrictive. See also #18159. I often write things like

    return S1.cardinality().factorial()

which would break if it was a Python integer.

OK, fair point.

And which infinity?

Is there not a corresponding test for infinite enumerated sets?

[behackl]

comment:15

Replying to @simon-king-jena:

I somehow disagree with that analysis that it is a problem that cardinality returns a Python integer. Yes, the category test suite verifies that cardinality returns a Sage integer. However (and that's why I Cc Nicolas) I don't think that the test should be so restrictive.

Hence, would it not be a better solution to modify the test so that it accepts both sage and python integers (and infinity)?

'Problem' meant that it is/was the reason for the failing doctest. Nevertheless, I'm also in favour of cardinality returning Integers. I also agree that we should change isinstance(card, Integer). But I'm not sure about returning infinity: within the category FiniteEnumeratedSets I don't think that cardinality should return infinity. Or should it?

In any case, should this be still part of this ticket?

[sagetrac-git]

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

59cb8d2

TESTS:: --> TESTS:

[sagetrac-git]

Changed commit from 5dfb836 to 59cb8d2

[behackl]

Reviewer: Benjamin Hackl

[behackl]

comment:17

I think it's best to open a separate ticket for the isinstance-issue (and that is what I also take from the silence here). I reviewed this and fixed a typo (TESTS:: --> TESTS: on one line).

This ticket fixes the issue with the (mysteriously) failing doctest in dyck_word.py: the changes pass make ptestlong on machines affected with the bug as well as on those not affected. Thus: positive_review.

[vbraun]

comment:18

Author name missing

[videlec]

Author: Vincent Delecroix

[videlec]

comment:19

Sorry!

[nthiery]

comment:20

Thanks for the fix!

While you are at it, you may consider making the assertion error even more explicit:

expected a Sage Integer and got {} of type {}

->

.cardinality() should return an Integer, and not {} of type {}

I let you decide whether you want to stick this in this ticket. If you do, you can reset a positive review on my behalf after the change (and running the tests).

[vbraun]

Changed branch from public/18244 to 59cb8d2