Opened 4 months ago

Closed 4 months ago

Last modified 4 months ago

#26431 closed enhancement (fixed)

py3: not sorting vertices on some graph functions

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.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) Commit: 51d798a87885f35ba8f55a69967a64221a1e9f10
Dependencies: Stopgaps:

Description


Change History (14)

comment:1 Changed 4 months ago by chapoton

  • Branch set to u/chapoton/26431
  • Commit set to d3b28ad93c02316dc4936f388ed885c255e6957c
  • Status changed from new to needs_review

New commits:

d3b28adpy3: not sorting vertices in spring layout and transitive reduction of graphs

comment:2 Changed 4 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:3 Changed 4 months ago by dcoudert

+1. Was about to give positive review as well ;)

comment:4 Changed 4 months ago by vbraun

  • Status changed from positive_review to needs_work

There are a bunch of numerical noise issues, see patchbot

comment:5 Changed 4 months ago by git

  • Commit changed from d3b28ad93c02316dc4936f388ed885c255e6957c to d89d32d4d5e6f04252f99e65f81c11f4c1bca08d

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

3c9db82Merge branch 'u/chapoton/26431' in 8.4
d89d32dtrac 26431 fixing doctest by flagging # random

comment:6 Changed 4 months ago by chapoton

  • Status changed from needs_work to needs_review

comment:7 follow-up: Changed 4 months ago by 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 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 ; follow-up: Changed 4 months ago by 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 ?

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 4 months ago by dcoudert

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

  • Commit changed from d89d32d4d5e6f04252f99e65f81c11f4c1bca08d to 51d798a87885f35ba8f55a69967a64221a1e9f10

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

51d798atrac 26431 better doctests

comment:11 Changed 4 months ago by chapoton

Comme ca, ca irait ?

comment:12 Changed 4 months ago by dcoudert

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, David Coudert
  • Status changed from needs_review to positive_review

Perfect.

comment:13 Changed 4 months ago by vbraun

  • Branch changed from u/chapoton/26431 to 51d798a87885f35ba8f55a69967a64221a1e9f10
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:14 Changed 4 months ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

This should be re-targeted for 8.5.

Note: See TracTickets for help on using tickets.