Opened 7 years ago

Closed 7 years ago

#18244 closed defect (fixed)

mysterious doctest failure on dyck_word.py

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-6.7
Component: documentation Keywords:
Cc: behackl, nthiery Merged in:
Authors: Vincent Delecroix Reviewers: Benjamin Hackl
Report Upstream: N/A Work issues:
Branch: 59cb8d2 (Commits, GitHub, GitLab) Commit: 59cb8d2ec7ee3710bf0c7b3e2948382279fa7469
Dependencies: Stopgaps:

Status badges

Description

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

Change History (21)

comment:1 Changed 7 years ago by vdelecroix

  • Branch set to public/18244
  • Commit set to 75f0a4a3305375b9bbad62aaadb22a95bd009c31

New commits:

75f0a4aTrac 18244: be more verbose in _test_enumerated_set_iter_cardinality

comment:2 Changed 7 years ago by behackl

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.

Last edited 7 years ago by behackl (previous) (diff)

comment:3 Changed 7 years ago by vdelecroix

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.

comment:4 Changed 7 years ago by vdelecroix

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!

comment:5 follow-up: Changed 7 years ago by behackl

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. ;-)

comment:6 Changed 7 years ago by git

  • Commit changed from 75f0a4a3305375b9bbad62aaadb22a95bd009c31 to 5dfb8363c6fe1920dbaa09e3bae1e8f5a76be34c

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

5dfb836Trac 18244: fix (indirectly) cardinality in dyck_word

comment:7 in reply to: ↑ 5 Changed 7 years ago by vdelecroix

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.

comment:8 follow-up: Changed 7 years ago by 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.

comment:9 Changed 7 years ago by behackl

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. :-)

comment:10 Changed 7 years ago by vdelecroix

  • Authors set to Vincent Delecroix
  • Status changed from new to needs_review

comment:11 in reply to: ↑ 8 ; follow-ups: Changed 7 years ago by SimonKing

  • Authors Vincent Delecroix deleted
  • Cc nthiery added

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)?

comment:12 in reply to: ↑ 11 ; follow-up: Changed 7 years ago by vdelecroix

Replying to SimonKing:

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

comment:13 Changed 7 years ago by SimonKing

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.

comment:14 in reply to: ↑ 12 Changed 7 years ago by SimonKing

Replying to vdelecroix:

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?

comment:15 in reply to: ↑ 11 Changed 7 years ago by behackl

Replying to SimonKing:

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?

comment:16 Changed 7 years ago by git

  • Commit changed from 5dfb8363c6fe1920dbaa09e3bae1e8f5a76be34c to 59cb8d2ec7ee3710bf0c7b3e2948382279fa7469

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

59cb8d2TESTS:: --> TESTS:

comment:17 Changed 7 years ago by behackl

  • Reviewers set to Benjamin Hackl
  • Status changed from needs_review to positive_review

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.

comment:18 Changed 7 years ago by vbraun

  • Status changed from positive_review to needs_work

Author name missing

comment:19 Changed 7 years ago by vdelecroix

  • Authors set to Vincent Delecroix
  • Status changed from needs_work to positive_review

Sorry!

comment:20 Changed 7 years ago by nthiery

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).

comment:21 Changed 7 years ago by vbraun

  • Branch changed from public/18244 to 59cb8d2ec7ee3710bf0c7b3e2948382279fa7469
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.