Opened 2 years ago

Closed 2 years ago

#30963 closed enhancement (fixed)

remove CombinatorialClass in graph_path.py

Reported by: Frédéric Chapoton Owned by:
Priority: major Milestone: sage-9.3
Component: graph theory Keywords:
Cc: Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 2ac6bde (Commits, GitHub, GitLab) Commit: 2ac6bde9f0e9458553ad15d2ed2ea71d3c439743
Dependencies: Stopgaps:

Status badges

Description

as part of #12913

Change History (12)

comment:1 Changed 2 years ago by Frédéric Chapoton

Branch: → u/chapoton/30963
Commit: → 26212a54a2062dae4984e44e413d54f6c865f191
Status: new → needs_review

New commits:

​26212a5remove CombinatorialClass in graph_path.py

comment:2 Changed 2 years ago by Travis Scrimshaw

Reviewers: → Travis Scrimshaw

Failures seem unrelated, but why did you get rid of this doctest?

  • src/sage/combinat/graph_path.py 

    diff --git a/src/sage/combinat/graph_path.py b/src/sage/combinat/graph_path.py
index dc2c7ce..620ef03 100644
    a b class GraphPaths_all(CombinatorialClass, GraphPaths_common): 
    239240
    240241            sage: G = DiGraph({1:[2,2,3], 2:[3,4], 3:[4], 4:[5,5]}, multiedges=True)
    241242            sage: p = GraphPaths(G)
    242             sage: p == loads(dumps(p))
    243             True
    244243        """
    245244        self.graph = g
     245        Parent.__init__(self, category=FiniteEnumeratedSets())
    246246
    247247    def __repr__(self):
    248248        """

comment:3 Changed 2 years ago by Frédéric Chapoton

Hello Travis,

failures on darwin patchbot are due to usage of python 3.9

failures on petitbonum are because of a pexpect problem with singular #30945

Here, I have a problem with loads-dumps, because I cannot use UniqueRepresentation, the input (a graph) being not compatible with that. Should I make a custom __eq__ and __ne__ to be able to restore this doctest ?

comment:4 in reply to:  3 Changed 2 years ago by Travis Scrimshaw

Replying to chapoton:

Here, I have a problem with loads-dumps, because I cannot use UniqueRepresentation, the input (a graph) being not compatible with that. Should I make a custom __eq__ and __ne__ to be able to restore this doctest ?

Yes, I think that is best. In fact, the lack of equality testing might end up being a backwards incompatible change (although I do see it as being unlikely). Probably also should have a __hash__ too...

comment:5 Changed 2 years ago by git

Commit: 26212a54a2062dae4984e44e413d54f6c865f191 → 6e4666b354e0f048c7f5c5f98b9a959f08e6b017

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

​6e4666badding __eq__ and __ne__ for GraphPaths pickling

comment:6 Changed 2 years ago by Frédéric Chapoton

ok, here is the equality test

comment:7 Changed 2 years ago by Travis Scrimshaw

Status: needs_review → positive_review

Thank you. LGTM.

comment:8 Changed 2 years ago by Frédéric Chapoton

one moment, I see that I forgot to put back all the loads-dumps doctests..

comment:9 Changed 2 years ago by git

Commit: 6e4666b354e0f048c7f5c5f98b9a959f08e6b017 → 2ac6bde9f0e9458553ad15d2ed2ea71d3c439743
Status: positive_review → needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

​2ac6bdeadding back loads-dumps

comment:10 Changed 2 years ago by Frédéric Chapoton

done. I also removed the .Facade() that I added before and was probably used wrongly. Back to needs review, sorry for that.

comment:11 Changed 2 years ago by Frédéric Chapoton

Status: needs_review → positive_review

given the green bot, I will allow mself to set this back to positive

comment:12 Changed 2 years ago by Volker Braun

Branch: u/chapoton/30963 → 2ac6bde9f0e9458553ad15d2ed2ea71d3c439743
Resolution: → fixed
Status: positive_review → closed
Note: See TracTickets for help on using tickets.