#26431 closed enhancement (fixed)
py3: not sorting vertices on some graph functions
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.5 
Component:  python3  Keywords:  
Cc:  dcoudert  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Travis Scrimshaw, David Coudert 
Report Upstream:  N/A  Work issues:  
Branch:  51d798a (Commits, GitHub, GitLab)  Commit:  51d798a87885f35ba8f55a69967a64221a1e9f10 
Dependencies:  Stopgaps: 
Description
Change History (14)
comment:1 Changed 3 years ago by
 Branch set to u/chapoton/26431
 Commit set to d3b28ad93c02316dc4936f388ed885c255e6957c
 Status changed from new to needs_review
comment:2 Changed 3 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
LGTM.
comment:3 Changed 3 years ago by
+1. Was about to give positive review as well ;)
comment:4 Changed 3 years ago by
 Status changed from positive_review to needs_work
There are a bunch of numerical noise issues, see patchbot
comment:5 Changed 3 years ago by
 Commit changed from d3b28ad93c02316dc4936f388ed885c255e6957c to d89d32d4d5e6f04252f99e65f81c11f4c1bca08d
comment:6 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:7 followup: ↓ 8 Changed 3 years ago by
By flagging the tests # random
, aren't we taking the risk of missing errors induced by other changes ? I agree that these tests are very sensitive.
I agree that for this test we know exactly the type of the vertices, but isn't it better to do:
 sage: sorted(D.keys()) == sorted(g) + sage: set(D.keys()) == set(g)
I don't know what is best, just asking.
comment:8 in reply to: ↑ 7 ; followup: ↓ 9 Changed 3 years ago by
Replying to dcoudert:
By flagging the tests
# random
, aren't we taking the risk of missing errors induced by other changes ? I agree that these tests are very sensitive.
I see no really better option.. One could maybe check that the result is a dictionary, with keys in G and values that are pairs of numbers ?
I agree that for this test we know exactly the type of the vertices, but isn't it better to do:
 sage: sorted(D.keys()) == sorted(g) + sage: set(D.keys()) == set(g)I don't know what is best, just asking.
In this precise case, we can sort the vertices, so no need to worry about python3.
comment:9 in reply to: ↑ 8 Changed 3 years ago by
Replying to chapoton:
Replying to dcoudert:
By flagging the tests
# random
, aren't we taking the risk of missing errors induced by other changes ? I agree that these tests are very sensitive.I see no really better option.. One could maybe check that the result is a dictionary, with keys in G and values that are pairs of numbers ?
We could may be have one such test just in case. I don't have better proposals.
comment:10 Changed 3 years ago by
 Commit changed from d89d32d4d5e6f04252f99e65f81c11f4c1bca08d to 51d798a87885f35ba8f55a69967a64221a1e9f10
Branch pushed to git repo; I updated commit sha1. New commits:
51d798a  trac 26431 better doctests

comment:11 Changed 3 years ago by
Comme ca, ca irait ?
comment:12 Changed 3 years ago by
 Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, David Coudert
 Status changed from needs_review to positive_review
Perfect.
comment:13 Changed 3 years ago by
 Branch changed from u/chapoton/26431 to 51d798a87885f35ba8f55a69967a64221a1e9f10
 Resolution set to fixed
 Status changed from positive_review to closed
comment:14 Changed 2 years ago by
 Milestone changed from sage8.4 to sage8.5
This should be retargeted for 8.5.
New commits:
py3: not sorting vertices in spring layout and transitive reduction of graphs