Opened 6 months ago

Closed 6 months ago

#26282 closed enhancement (fixed)

Avoid comparison of vertex labels in MIP (Step 2)

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

Description

Avoid comparison of vertex labels in graph.py.

Change History (11)

comment:1 Changed 6 months ago by dcoudert

  • Branch set to public/26282_avoid_comparison_of_vertex_labels
  • Commit set to 8e3e85cff2110f86cecc13f75694d984188c5a08
  • Status changed from new to needs_review

New commits:

8e3e85ctrac #26282: avoid comparison of vertex labels in MIP

comment:2 Changed 6 months ago by git

  • Commit changed from 8e3e85cff2110f86cecc13f75694d984188c5a08 to 5221e6aa674537650478cdf06e3a98d2b19ec5a0

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

5221e6atrac #26282: resolve merge conflict with 8.4.beta5

comment:3 Changed 6 months ago by git

  • Commit changed from 5221e6aa674537650478cdf06e3a98d2b19ec5a0 to 2dfc524918880059b9d1ae08fc5b490d4a6e5c06

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

2dfc524trac #26282: use def instead of lambdas

comment:4 Changed 6 months ago by dcoudert

I change frozenset wrappers from lambda to def. Should I try to do the same for remaining lambda statements like this ?

weight = lambda x: x if x in RR else 1

comment:5 Changed 6 months ago by dcoudert

  • Cc tscrim chapoton added

comment:6 Changed 6 months ago by tscrim

I think you can get rid of

        def B(e):
            return b[frozenset(e)]
        def R(e):
            return r[frozenset(e)]
        def S(e):
            return frozenset(e)

as they are not simplifying the code (or changing the readability IMO.

I would still use six.iteritems for better memory usage:

-        g_mad = g.subgraph([v for v,l in six.iteritems(p.get_values(d)) if l>m ])
+        d_val = p.get_values(d)
+        g_mad = g.subgraph([v for v,l in d_val.items() if l > m ])

In general, I don't get the point of that change.

More standard convention and faster:

-lists = dict((v, []) for v in self)
+lists = {v: [] for v in self}
sage: %timeit dict((i,-1) for i in range(100))
100000 loops, best of 3: 16.6 µs per loop
sage: %timeit {i:-1 for i in range(100)}
100000 loops, best of 3: 11.7 µs per loop

not fuv in L -> fuv not in L.

comment:7 Changed 6 months ago by git

  • Commit changed from 2dfc524918880059b9d1ae08fc5b490d4a6e5c06 to 6acb781fc22f76e5cf544fe68daefb0c1a3c03c4

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

7856e11trac #: Merged with 8.4.beta7
6acb781trac #26282: implement reviewer's comments

comment:8 Changed 6 months ago by dcoudert

I have implemented all your comments.

comment:9 Changed 6 months ago by tscrim

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

Thanks. LGTM.

comment:10 Changed 6 months ago by dcoudert

Thank you !

comment:11 Changed 6 months ago by vbraun

  • Branch changed from public/26282_avoid_comparison_of_vertex_labels to 6acb781fc22f76e5cf544fe68daefb0c1a3c03c4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.