#21607 closed defect (fixed)
Posets: with_linear_extension() and wrong constructor
Reported by: | jmantysalo | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.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, GitHub, GitLab) | 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 6 years ago by
- Component changed from PLEASE CHANGE to combinatorics
- Type changed from PLEASE CHANGE to defect
comment:2 Changed 6 years ago by
- Branch set to u/jmantysalo/with-linear
comment:3 Changed 6 years ago by
- Cc chapoton added
- Commit set to d86222b4dede22ab2d9a321f875713db9b218cf8
- Status changed from new to needs_review
comment:4 follow-up: ↓ 6 Changed 6 years ago by
This is completely non-future-proof. 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 6 years ago by
- Status changed from needs_review to needs_work
comment:6 in reply to: ↑ 4 Changed 6 years ago by
- Cc nthiery added
CC to Nicolas, as the question is more general.
Replying to tscrim:
This is completely non-future-proof. 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 sub-sub-classes 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 6 years ago by
- Commit changed from d86222b4dede22ab2d9a321f875713db9b218cf8 to 07f7f869b06eecdeb375873690a8899daeafd063
Branch pushed to git repo; I updated commit sha1. New commits:
07f7f86 | A non-working example.
|
comment:8 Changed 6 years ago by
Here is an example with __class__
. It fails for promotion()
and evacuation()
.
comment:9 follow-up: ↓ 10 Changed 6 years ago by
- Branch changed from u/jmantysalo/with-linear to public/posets/with_linear_extension-21607
- 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 ; follow-up: ↓ 13 Changed 6 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 6 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 6 years ago by
trac bug.
comment:13 in reply to: ↑ 10 Changed 6 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 6 years ago by
- Commit changed from 971067de7825fa5a5ddb99ee43b3688dee2c3b3a to 9a9126efb068798c9796bc8d5890f9e9a1d80ee8
comment:15 Changed 6 years ago by
- Milestone changed from sage-7.4 to sage-7.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 6 years ago by
- Branch changed from public/posets/with_linear_extension-21607 to 9a9126efb068798c9796bc8d5890f9e9a1d80ee8
- Resolution set to fixed
- Status changed from positive_review to closed
comment:17 Changed 5 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 non-related docstrings.
New commits:
Correction to with_linear_extension + some more.