#25259 closed defect (fixed)

py3: buffet of minor fixes involving dict iterators

Reported by: embray Owned by:
Priority: major Milestone: sage-8.3
Component: python3 Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Travis Scrimshaw, Frédéric Chapoton, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 9d0bfa4 (Commits) Commit: 9d0bfa425e1466158e25d1ab6b7cee8add4d07c3
Dependencies: Stopgaps:

Description

This fixes several doctests, and a few APIs that involved returning dict iterators from dict.keys(), dict.values(), or dict.items(). All existing code assumes that these methods return lists containing copies of data from the dict, whereas on Python 3 they return live views into the dict, which makes for some rather different behavior (and in some cases different doctest output).

This probably isn't exhaustive, but is the majority of such bugs I could find in a recent run of the test suite on Python 3.

Change History (24)

comment:1 Changed 18 months ago by embray

  • Status changed from new to needs_review

comment:2 follow-ups: Changed 18 months ago by tscrim

At least using Python2 it is faster to do list(D.items())

sage: import six
sage: D = {i:i for i in range(10)}
sage: %timeit list(six.iteritems(D))
The slowest run took 88.95 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 1.5 µs per loop
sage: %timeit list(D.items())
1000000 loops, best of 3: 1.11 µs per loop
sage: D = {i:i for i in range(100)}
sage: %timeit list(six.iteritems(D))
100000 loops, best of 3: 7.27 µs per loop
sage: %timeit list(D.items())
100000 loops, best of 3: 6.65 µs per loop

Granted, it is not necessarily memory efficient, but I think speed in functor.pyx is more important. Same comment can be applied for the other similar changes, including values().

Why do you still use keys() in generic_graph.py?

In matrix_generic_sparse.pyx, I don't think you need the list:

for key in list(entries):

Similar for reset.pyx, conway_polynomials.py, and polydict.pyx. Likewise, I don't understand the need for the change in decorators.py.

comment:3 Changed 18 months ago by embray

I don't really care about such minor differences unless it can be shown to have an impact on the specific code in question (e.g. that code itself is in a large loop.

comment:4 in reply to: ↑ 2 Changed 18 months ago by embray

Replying to tscrim:

Why do you still use keys() in generic_graph.py?

No specific reason--I guess i wasn't sure if the return value there was actually a dict or not, but it's harmless either way. In this case I think having the .keys() is almost better from a documentation perspective.

In matrix_generic_sparse.pyx, I don't think you need the list:

for key in list(entries):

Similar for reset.pyx, conway_polynomials.py, and polydict.pyx. Likewise, I don't understand the need for the change in decorators.py.

You do--in all those cases the dict being looped over is modified in the loops so you must make a list first.

comment:5 in reply to: ↑ 2 Changed 18 months ago by embray

Replying to tscrim:

Granted, it is not necessarily memory efficient, but I think speed in functor.pyx is more important. Same comment can be applied for the other similar changes, including values().

The change in functor.pyx is to its __reduce__ method. Is there really a reason that's critical? Some difference like this pales in comparison to the overall process of creating a pickle from an object.

comment:6 Changed 18 months ago by chapoton

  • Branch changed from u/embray/python3/misc/dict-iters to public/25259
  • Commit changed from 2cef71b11cd36fcc8bb5b6603451da721497bb30 to 855d7018ba011d6972d8cc948fbe3807870b6ab4

rebased on 8.3.b0


New commits:

855d701Merge branch 'u/embray/python3/misc/dict-iters' in 8.3.b0

comment:7 Changed 18 months ago by chapoton

This introduces bad uses of six inside some pyx files:

  • src/sage/categories/functor.pyx
  • src/sage/combinat/designs/designs_pyx.pyx

comment:8 Changed 18 months ago by tscrim

Thanks for the explanations in comment:4 (I had missed the modified dictionaries). However, we should remove the use of six in pyx files as mentioned in comment:7.

comment:9 Changed 18 months ago by git

  • Commit changed from 855d7018ba011d6972d8cc948fbe3807870b6ab4 to 1a73750831b692159bdb041062e949965dab2539

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

1a73750trac 25259 do not use six in pyx files

comment:10 Changed 18 months ago by chapoton

I have taken care of the removal of six from pyx files

comment:11 Changed 18 months ago by chapoton

  • Milestone changed from sage-8.2 to sage-8.3

comment:12 Changed 18 months ago by tscrim

  • Reviewers set to Travis Scrimshaw, Frédéric Chapoton
  • Status changed from needs_review to positive_review

LGTM. Thanks.

comment:13 Changed 18 months ago by embray

Ok but I'm going to clean up / squash the commit history.

comment:14 Changed 18 months ago by git

  • Commit changed from 1a73750831b692159bdb041062e949965dab2539 to ae64661c54f8caa06d6039c698527fcf1f302df0
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

c8ebd95py3: numerous fixes to code involving dict iterators (.keys(), .values(), .items()
ae64661py3: fix several tests broken on Python 3 that involved dictionary iterators in some way, or in particular did not have a guaranteed ordering in the output due to dictionary iteration

comment:15 Changed 17 months ago by chapoton

does not merge, needs rebase

comment:16 Changed 17 months ago by jdemeyer

  • Status changed from needs_review to needs_work

Merge conflict

comment:17 Changed 17 months ago by git

  • Commit changed from ae64661c54f8caa06d6039c698527fcf1f302df0 to 65b95d76ce2bc99a6d9b92f588e432a3ac583938

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

65b95d7Merge branch 'public/25259' of trac.sagemath.org:sage into 8.3.b1

comment:18 Changed 17 months ago by chapoton

  • Status changed from needs_work to needs_review

merged

comment:19 Changed 17 months ago by jdemeyer

  • Reviewers changed from Travis Scrimshaw, Frédéric Chapoton to Travis Scrimshaw, Frédéric Chapoton, Jeroen Demeyer

comment:20 Changed 17 months ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:21 Changed 17 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:22 Changed 17 months ago by git

  • Commit changed from 65b95d76ce2bc99a6d9b92f588e432a3ac583938 to 9d0bfa425e1466158e25d1ab6b7cee8add4d07c3

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

9d0bfa4Merge branch 'public/25259' in 8.3.b2

comment:23 Changed 17 months ago by chapoton

  • Status changed from needs_work to positive_review

merged. Setting back to positive

comment:24 Changed 17 months ago by vbraun

  • Branch changed from public/25259 to 9d0bfa425e1466158e25d1ab6b7cee8add4d07c3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.