Skip to content
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

Bug in Poset.is_slender(?) #22373

Closed
jdemeyer opened this issue Feb 12, 2017 · 18 comments
Closed

Bug in Poset.is_slender(?) #22373

jdemeyer opened this issue Feb 12, 2017 · 18 comments

Comments

@jdemeyer
Copy link

CC: @mezzarobba @jm58660

Component: combinatorics

Author: Marc Mezzarobba, Jori Mäntysalo, Frédéric Chapoton

Branch/Commit: 7608202

Reviewer: Jori Mäntysalo, Frédéric Chapoton

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

@jdemeyer jdemeyer added this to the sage-7.6 milestone Feb 12, 2017
@jdemeyer
Copy link
Author

@fchapoton
Copy link
Contributor

comment:2

iteritems is not python3-compatible

you should rather use

from six import iteritems
for xx in iteritems(dd):
    pass

New commits:

90a31a1Bug in Poset.is_slender(?)

@fchapoton
Copy link
Contributor

Commit: 90a31a1

@mezzarobba
Copy link
Member

@mezzarobba
Copy link
Member

Changed commit from 90a31a1 to 4796945

@mezzarobba
Copy link
Member

comment:4

Replying to @fchapoton:

iteritems is not python3-compatible

Fixed, thanks!

Without this patch, #22029 made one of the doctests of is_slender() fail.
Jori, can you tell if there was indeed a bug and perhaps how to add a test for the fix that would work even without #22029? Thanks!


New commits:

4796945Bug in Poset.is_slender(?)

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Feb 15, 2017

comment:5

Replying to @mezzarobba:

Without this patch, #22029 made one of the doctests of is_slender() fail.
Jori, can you tell if there was indeed a bug and perhaps how to add a test for the fix that would work even without #22029?

What was the failing example?

I would guess that the other patch shows some hidden bug in an example poset. That should be easy to spot, just make all the example posets with and without the patch and try

L.hasse_diagram().canonical_label().dig6_string()

@mezzarobba
Copy link
Member

comment:6

Replying to @jm58660:

What was the failing example?

It must have been the one with certificate=True, since my patch only touches the certificate. But I don't remember exactly which changes elsewhere made it fail.

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Feb 16, 2017

comment:7

Yes, I have been an idiot when doing the certificate-code. Test code:

set_random_seed(332211)
D = Posets.DiamondPoset(5)._hasse_diagram

for i in range(100):
    P = Posets.RandomPoset(randint(10, 20), 0.2)
    if P.is_slender():
        if P._hasse_diagram.subgraph_search(D) is not None:
            print("Bug 1")
            break
    else:
        if P._hasse_diagram.subgraph_search(D) is None:
            print("Bug 2")
            break
        _, cert = P.is_slender(certificate=True)
        P_ = P.subposet(P.interval(cert[0], cert[1]))
        if P_.height() != 3:
            print("Bug 3")
            break
        if P_.cardinality() < 5:
            print("Bug 4")
            break
else:
    print("All OK")

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Feb 16, 2017

comment:8

How should we define this to be on

LatticePoset({0: [1, 2, 3, 4], 1: [6], 2: [6], 3: [6], 4: [5], 5: [6]})

? I think that my test code has error, when I compare it to the definition in the docstring.

@fchapoton
Copy link
Contributor

Changed branch from u/mmezzarobba/22373-is_slender to public/22373

@fchapoton
Copy link
Contributor

Changed commit from 4796945 to 7608202

@fchapoton
Copy link
Contributor

comment:9

I added a doctest, currently return the wrong answer 5.

I also changed six.iteritems to iteritems in the full file.


New commits:

6feb62fMerge branch 'u/mmezzarobba/22373-is_slender' in 7.6.rc0
7608202trac 22373 adding a doctest

@fchapoton
Copy link
Contributor

comment:10

ok, green bot. Looks good to me. Jori, if you agree, you can set to positive.

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Mar 16, 2017

comment:11

Replying to @fchapoton:

ok, green bot. Looks good to me. Jori, if you agree, you can set to positive.

Yes, seems to be OK.

@mezzarobba
Copy link
Member

Changed author from Marc Mezzarobba to Marc Mezzarobba, Jori Mäntysalo, Frédéric Chapoton

@mezzarobba
Copy link
Member

Reviewer: Jori Mäntysalo, Frédéric Chapoton

@vbraun
Copy link
Member

vbraun commented Mar 27, 2017

Changed branch from public/22373 to 7608202

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants