#21607 closed defect (fixed)
Posets: with_linear_extension() and wrong constructor
Reported by:  jmantysalo  Owned by:  

Priority:  major  Milestone:  sage7.5 
Component:  combinatorics  Keywords:  
Cc:  chapoton, nthiery  Merged in:  
Authors:  Jori Mäntysalo, Travis Scrimshaw  Reviewers:  Travis Scrimshaw, Nicolas M. Thiéry 
Report Upstream:  N/A  Work issues:  
Branch:  9a9126e (Commits)  Commit:  
Dependencies:  Stopgaps: 
Description
sage: P = Posets.PentagonPoset() sage: type(P), P.category() (<class 'sage.combinat.posets.lattices.FiniteLatticePoset_with_category'>, Join of Category of finite lattice posets . . . sage: P_ = P.with_linear_extension([0, 1, 3, 2, 4]) sage: type(P_), P_.category() (<class 'sage.combinat.posets.posets.FinitePoset_with_category'>, Join of Category of finite lattice posets . . .
and so
sage: P.meet_irreducibles(), P_.meet_irreducibles() ([1, 2, 3], [1, 3, 2])
but
sage: P.double_irreducibles() [1, 2, 3] sage: P_.double_irreducibles() AttributeError Traceback (most recent call last)
Change History (17)
comment:1 Changed 4 years ago by
 Component changed from PLEASE CHANGE to combinatorics
 Type changed from PLEASE CHANGE to defect
comment:2 Changed 4 years ago by
 Branch set to u/jmantysalo/withlinear
comment:3 Changed 4 years ago by
 Cc chapoton added
 Commit set to d86222b4dede22ab2d9a321f875713db9b218cf8
 Status changed from new to needs_review
comment:4 followup: ↓ 6 Changed 4 years ago by
This is completely nonfutureproof. A better solution would be to use self.__class__
or something to this affect. I'm still 1 on removing ``self``
from docstrings.
comment:5 Changed 4 years ago by
 Status changed from needs_review to needs_work
comment:6 in reply to: ↑ 4 Changed 4 years ago by
 Cc nthiery added
CC to Nicolas, as the question is more general.
Replying to tscrim:
This is completely nonfutureproof. A better solution would be to use
self.__class__
or something to this affect.
Nope. Only really foolproof solution is to have base class A
to never know anything about subclasses, only give hooks for them. A would have something like
def _give_f_constructor(self): return A def f(): . . . something here, for example, creates X . . . constructor = self._give_f_constructor() return constructor(X)
and then B might have
def _give_f_constructor(self): return B
It is not possible for a class A
to know if somebody will add subsubclasses B > C > D
and so on, so that for example D.f()
should return a type of C
. Complement of a bipartite graph was an example of this.
But in reality we can't achieve that. So, I can make the construction with __class__
. But if the category system gives some solution to this problem, I will hear.
I'm still 1 on removing
``self``
from docstrings.
Yeah, should be resolved in a way or another. Belongs to the same class than "certificate=" vs. "certify=", "algorithm=" vs. "implementation=" etc.
comment:7 Changed 4 years ago by
 Commit changed from d86222b4dede22ab2d9a321f875713db9b218cf8 to 07f7f869b06eecdeb375873690a8899daeafd063
Branch pushed to git repo; I updated commit sha1. New commits:
07f7f86  A nonworking example.

comment:8 Changed 4 years ago by
Here is an example with __class__
. It fails for promotion()
and evacuation()
.
comment:9 followup: ↓ 10 Changed 4 years ago by
 Branch changed from u/jmantysalo/withlinear to public/posets/with_linear_extension21607
 Commit changed from 07f7f869b06eecdeb375873690a8899daeafd063 to 971067de7825fa5a5ddb99ee43b3688dee2c3b3a
 Reviewers set to Travis Scrimshaw
 Status changed from needs_work to needs_review
It is a subtle problem with UniqueRepresentation
in that the class itself is part of the key for the cache. So what we have to do is pull out the actual class instead of the one created by the category framework. The category framework guarantees that the original class is in position 1 in the MRO (otherwise, the category methods would override the concrete classes), so this is safe to do.
New commits:
971067d  Get the actual class, not the one created by the category, and some other tweaks.

comment:10 in reply to: ↑ 9 ; followup: ↓ 13 Changed 4 years ago by
Replying to tscrim:
It is a subtle problem with
UniqueRepresentation
in that the class itself is part of the key for the cache. So what we have to do is pull out the actual class instead of the one created by the category framework. The category framework guarantees that the original class is in position 1 in the MRO (otherwise, the category methods would override the concrete classes), so this is safe to do.
OK. Hope that Frédéric or Nicolas can review this, as I don't understand the category system.
Now, for example relabel()
contains code block starting
if isinstance(self, FiniteLatticePoset): constructor = FiniteLatticePoset elif isinstance(self, FiniteMeetSemilattice): constructor = FiniteMeetSemilattice
Should we change that too?
comment:11 Changed 4 years ago by
Now clicking branch shows whole posets.py
in red. Is this just a bug of Trac, or is there some real errors, conflicts or something?
comment:12 Changed 4 years ago by
trac bug.
comment:13 in reply to: ↑ 10 Changed 4 years ago by
Replying to jmantysalo:
OK. Hope that Frédéric or Nicolas can review this, as I don't understand the category system.
Ping. Travis can not be the only one who knows what self.__class__.__mro__[1]
does and if can got broken or not.
comment:14 Changed 4 years ago by
 Commit changed from 971067de7825fa5a5ddb99ee43b3688dee2c3b3a to 9a9126efb068798c9796bc8d5890f9e9a1d80ee8
comment:15 Changed 4 years ago by
 Milestone changed from sage7.4 to sage7.5
 Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Nicolas Thiéry
 Status changed from needs_review to positive_review
Travis said that Nicolas has checked the mro
line, and so I mark this as positive review.
comment:16 Changed 4 years ago by
 Branch changed from public/posets/with_linear_extension21607 to 9a9126efb068798c9796bc8d5890f9e9a1d80ee8
 Resolution set to fixed
 Status changed from positive_review to closed
comment:17 Changed 3 years ago by
 Commit 9a9126efb068798c9796bc8d5890f9e9a1d80ee8 deleted
 Reviewers changed from Travis Scrimshaw, Nicolas Thiéry to Travis Scrimshaw, Nicolas M. Thiéry
The patch contains also slight modifications to nonrelated docstrings.
New commits:
Correction to with_linear_extension + some more.