Opened 12 months ago

Closed 12 months ago

#26528 closed enhancement (fixed)

avoid using .vertices() in comparability, hyperbolicity and distances_all_pairs

Reported by: dcoudert Owned by:
Priority: major Milestone: sage-8.5
Component: graph theory Keywords: py3, graph
Cc: tscrim, chapoton Merged in:
Authors: David Coudert Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 564cb70 (Commits) Commit: 564cb700c9d5e3ca4c4689dc25006a651d76f2bc
Dependencies: Stopgaps:

Description

We expose the new parameter vertex_list of init_short_digraph in methods of distances_all_pairs.pyx and so

  • add the same parameter to methods of distances_all_pairs.pyx
  • remove the use of .vertices() in hyperbolicity.pyx and comparability.pyx

We could do the same in bandwidth.pyx but this might be in conflict with #26520

Several use of .vertices() remain in distances_all_pairs.pyx that are harder to remove.

Change History (8)

comment:1 Changed 12 months ago by dcoudert

  • Branch set to public/26528_reduce_use_of_vertices
  • Cc tscrim chapoton added
  • Commit set to 90c83bc872f8969c016bf8956f26be4bac0aaa0e
  • Status changed from new to needs_review

Hope you like it ;)

New commits:

​90c83bctrac #26528: reduce use of .vertices() in distances_all_pairs

comment:2 Changed 12 months ago by tscrim

I am not convinced that the test for is_transitive is always going to give the same answer: 

         sage: digraphs.DeBruijn(5,2).is_transitive(certificate=True)
-        ('00', '10')
+        ('22', '02')

I think a more robust test would be to verify this by: 

sage: G = digraphs.DeBruijn(5,2)
sage: cert = G.is_transitive(certificate=True)
sage: G.has_edge(*cert)
False
sage: G.shortest_path(*cert) != []
True

comment:3 Changed 12 months ago by tscrim

Also, I think it would be better to do `0..n-1` -> `(0, \ldots, n-1)`. Moreover, you need to change `i`th -> `i`-th because Sphinx would not treat the closing backtick like it should.

Otherwise LGTM.

comment:4 Changed 12 months ago by tscrim

  • Reviewers set to Travis Scrimshaw

comment:5 Changed 12 months ago by git

  • Commit changed from 90c83bc872f8969c016bf8956f26be4bac0aaa0e to 564cb700c9d5e3ca4c4689dc25006a651d76f2bc

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

​564cb70trac #26528: reviewer's comments

comment:6 Changed 12 months ago by dcoudert

Thanks for the comments.

comment:7 Changed 12 months ago by tscrim

  • Status changed from needs_review to positive_review

Thanks. LGTM.

comment:8 Changed 12 months ago by vbraun

  • Branch changed from public/26528_reduce_use_of_vertices to 564cb700c9d5e3ca4c4689dc25006a651d76f2bc
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.