Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#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) 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 jmantysalo

  • Component changed from PLEASE CHANGE to combinatorics
  • Type changed from PLEASE CHANGE to defect

comment:2 Changed 4 years ago by jmantysalo

  • Branch set to u/jmantysalo/with-linear

comment:3 Changed 4 years ago by jmantysalo

  • Authors set to Jori Mäntysalo
  • Cc chapoton added
  • Commit set to d86222b4dede22ab2d9a321f875713db9b218cf8
  • Status changed from new to needs_review

The patch contains also slight modifications to non-related docstrings.


New commits:

d86222bCorrection to with_linear_extension + some more.

comment:4 follow-up: Changed 4 years ago by tscrim

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 4 years ago by tscrim

  • Status changed from needs_review to needs_work

comment:6 in reply to: ↑ 4 Changed 4 years ago by jmantysalo

  • 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 4 years ago by git

  • Commit changed from d86222b4dede22ab2d9a321f875713db9b218cf8 to 07f7f869b06eecdeb375873690a8899daeafd063

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

07f7f86A non-working example.

comment:8 Changed 4 years ago by jmantysalo

Here is an example with __class__. It fails for promotion() and evacuation().

comment:9 follow-up: Changed 4 years ago by tscrim

  • 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:

971067dGet the actual class, not the one created by the category, and some other tweaks.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 4 years ago by jmantysalo

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 jmantysalo

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 tscrim

trac bug.

comment:13 in reply to: ↑ 10 Changed 4 years ago by jmantysalo

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 git

  • Commit changed from 971067de7825fa5a5ddb99ee43b3688dee2c3b3a to 9a9126efb068798c9796bc8d5890f9e9a1d80ee8

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

334aab9Merge branch 'public/posets/with_linear_extension-21607' of trac.sagemath.org:sage into public/posets/with_linear_extension-21607
9a9126eUsing __base__ instead of __mro__[1].

comment:15 Changed 4 years ago by jmantysalo

  • Authors changed from Jori Mäntysalo to Jori Mäntysalo, Travis Scrimshaw
  • 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 4 years ago by vbraun

  • 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 3 years ago by jdemeyer

  • Commit 9a9126efb068798c9796bc8d5890f9e9a1d80ee8 deleted
  • Reviewers changed from Travis Scrimshaw, Nicolas Thiéry to Travis Scrimshaw, Nicolas M. Thiéry
Note: See TracTickets for help on using tickets.