Opened 2 years ago

Closed 2 years ago

#24068 closed enhancement (fixed)

py3: some care for .values()[...]

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.1
Component: python3 Keywords:
Cc: Merged in:
Authors: Frédéric Chapoton Reviewers: Jeroen Demeyer, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 67468c2 (Commits) Commit: 67468c2baddbeb7212c40d971e1c18d1eec2afdb
Dependencies: Stopgaps:

Description

part of #15981

Change History (18)

comment:1 Changed 2 years ago by chapoton

  • Branch set to u/chapoton/24068
  • Commit set to 010fb5d8fd8febed33e4cdcbc994420c31194d8c
  • Status changed from new to needs_review

New commits:

010fb5dpy3 : some care for .values()[something]

comment:2 Changed 2 years ago by git

  • Commit changed from 010fb5d8fd8febed33e4cdcbc994420c31194d8c to bfc9b7ab96707d85ae8a0d7e22021929fb537d2e

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

bfc9b7atrac 24068 missing imports added

comment:3 Changed 2 years ago by tscrim

For this change:

  • src/sage/combinat/combinat.py

    diff --git a/src/sage/combinat/combinat.py b/src/sage/combinat/combinat.py
    index 9602889..1cdb339 100644
    a b Functions and classes 
    144144#*****************************************************************************
    145145from __future__ import absolute_import
    146146from six.moves import range
    147 from six import iteritems, add_metaclass
     147from six import iteritems, add_metaclass, itervalues
    148148
    149149from sage.interfaces.all import maxima
    150150from sage.rings.all import ZZ, QQ, Integer, infinity
    class CombinatorialElement(CombinatorialObject, Element): 
    12931293        if len(args) == 1 and not kwds:
    12941294            L = args[0]
    12951295        elif len(kwds) == 1 and not args:
    1296             L = kwds.values()[0]
     1296            L = next(itervalues(kwds))
    12971297        else:
    12981298            raise TypeError("__init__() takes exactly 2 arguments ({} given)".format(1+len(args)+len(kwds)))
    12991299        super(CombinatorialElement, self).__init__(L)

we should be able to do L, = kwds.values().

comment:4 Changed 2 years ago by git

  • Commit changed from bfc9b7ab96707d85ae8a0d7e22021929fb537d2e to 2c2da594b397b22cf396b9c679fb015ae7204794

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

2c2da59trac 24068 fixes

comment:5 Changed 2 years ago by git

  • Commit changed from 2c2da594b397b22cf396b9c679fb015ae7204794 to ce14978add12c2b34edd6f720dde2abf15c25371

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

ce14978trac 24068 fix

comment:6 Changed 2 years ago by jdemeyer

Sometimes you use iter(x.values()) and sometimes itervalues(x). Wouldn't it be better to always use six.itervalues?

comment:7 follow-up: Changed 2 years ago by chapoton

I only used iter(x.values()) when the other option was not working.

comment:8 in reply to: ↑ 7 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Replying to chapoton:

I only used iter(x.values()) when the other option was not working.

Because you are changing the values() method for a non-dict!

comment:9 Changed 2 years ago by git

  • Commit changed from ce14978add12c2b34edd6f720dde2abf15c25371 to c89ef05775f41c7f5230f8a8a0ae212283ac0a44

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

c89ef05trac 24068 undo wrong change

comment:10 Changed 2 years ago by chapoton

  • Status changed from needs_work to needs_review

indeed. Corrected

comment:11 Changed 2 years ago by chapoton

green bot now at last

comment:12 Changed 2 years ago by chapoton

ping?

comment:13 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

I disagree with

  • src/sage/graphs/generic_graph.py

    diff --git a/src/sage/graphs/generic_graph.py b/src/sage/graphs/generic_graph.py
    index c3d5fe5..d5d6c5a 100644
    a b class GenericGraph(GenericGraph_pyx): 
    1310113101        From this embedding, we can clearly build an interval graph
    1310213102        isomorphic to the previous one::
    1310313103
    13104             sage: g2 = graphs.IntervalGraph(d.values())
     13104            sage: g2 = graphs.IntervalGraph(list(d.values()))
    1310513105            sage: g2.is_isomorphic(g)
    1310613106            True

Instead, I suggest

  • src/sage/graphs/generators/intersection.py

    diff --git a/src/sage/graphs/generators/intersection.py b/src/sage/graphs/generators/intersection.py
    index a81c4a6..ba5b6a7 100644
    a b def IntervalGraph(intervals, points_ordered = False): 
    9090        sage: g.edges() == h.edges()
    9191        True
    9292    """
     93    intervals = list(intervals)
    9394
    9495    n = len(intervals)
    9596    g = Graph(n)

In src/sage/rings/polynomial/multi_polynomial_ring.py, some indentation is wrong (not a multiple of 4). Since you're change that code anyway, better fix that.

In src/sage/sets/set_from_iterator.py, you can remove the assert since the new code already checks that.

That's all.

comment:14 Changed 2 years ago by git

  • Commit changed from c89ef05775f41c7f5230f8a8a0ae212283ac0a44 to 67468c2baddbeb7212c40d971e1c18d1eec2afdb

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

35f5a95Merge branch 'u/chapoton/24068' in 8.1.b9
67468c2trac 24068 some details

comment:15 Changed 2 years ago by chapoton

  • Status changed from needs_work to needs_review

Thanks. All done

comment:16 Changed 2 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer

positive review if the patchbot passes.

comment:17 Changed 2 years ago by tscrim

  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Travis Scrimshaw
  • Status changed from needs_review to positive_review

comment:18 Changed 2 years ago by vbraun

  • Branch changed from u/chapoton/24068 to 67468c2baddbeb7212c40d971e1c18d1eec2afdb
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.