Sage: Ticket #25948: py3: a few more miscellaneous dict iterator (dict.keys, dict.values) fixes
https://trac.sagemath.org/ticket/25948
<p>
As in <a class="closed ticket" href="https://trac.sagemath.org/ticket/25947" title="#25947: defect: py3: miscellaneous map -> list fixes (closed: fixed)">#25947</a>, found by grepping through the test output. A couple of these are far-reaching.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/25948
Trac 1.2Erik BrayFri, 27 Jul 2018 11:50:23 GMTstatus changed
https://trac.sagemath.org/ticket/25948#comment:1
https://trac.sagemath.org/ticket/25948#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
TicketTravis ScrimshawFri, 27 Jul 2018 12:42:34 GMTkeywords, reviewer set
https://trac.sagemath.org/ticket/25948#comment:2
https://trac.sagemath.org/ticket/25948#comment:2
<ul>
<li><strong>keywords</strong>
<em>sagedays@icerm</em> added
</li>
<li><strong>reviewer</strong>
set to <em>Travis Scrimshaw</em>
</li>
</ul>
<p>
I think it would be better to do <code>list(d.values())</code> than <code>list(itervalues(d))</code>, both because it means less <code>six</code> dependence (which means it looks like Python3 in terms of the code) and for speed in Python2:
</p>
<pre class="wiki">sage: from six import itervalues
sage: d = {i:i for i in range(100)}
sage: %timeit list(itervalues(d))
The slowest run took 33.55 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 1.34 µs per loop
sage: %timeit list(d.values())
The slowest run took 13.27 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 899 ns per loop
</pre><p>
Given that these lists are likely small (less than 100, probably averaging between 5-10), I think any memory duplication is negligible.
</p>
<p>
Otherwise LGTM.
</p>
TicketgitFri, 27 Jul 2018 13:29:27 GMTcommit changed
https://trac.sagemath.org/ticket/25948#comment:3
https://trac.sagemath.org/ticket/25948#comment:3
<ul>
<li><strong>commit</strong>
changed from <em>ce5c10b4ea36729f4fa3d916e982e72df394cc4b</em> to <em>75e0d2c3ac82d895d9153830490fd7f162173dc1</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=5973ae603ce326cf42d76108b0286d0ecf350803"><span class="icon"></span>5973ae6</a></td><td><code>py3: a couple more minor dict iterator fixes</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=fa50082ba156219e581f58fc90ad1a4bb20712df"><span class="icon"></span>fa50082</a></td><td><code>py3: return lists instead of dict iterators for these methods</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=664a969284b95766340df92a5f4307289cab2865"><span class="icon"></span>664a969</a></td><td><code>py3: more minor fixes involving dict iterators and dict sorting</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=75e0d2c3ac82d895d9153830490fd7f162173dc1"><span class="icon"></span>75e0d2c</a></td><td><code>py3: one more dict.keys -> list fix</code>
</td></tr></table>
TicketErik BrayFri, 27 Jul 2018 13:29:54 GMT
https://trac.sagemath.org/ticket/25948#comment:4
https://trac.sagemath.org/ticket/25948#comment:4
<p>
Added a few more; still all trivial one-liners I think.
</p>
TicketgitFri, 27 Jul 2018 13:58:16 GMTcommit changed
https://trac.sagemath.org/ticket/25948#comment:5
https://trac.sagemath.org/ticket/25948#comment:5
<ul>
<li><strong>commit</strong>
changed from <em>75e0d2c3ac82d895d9153830490fd7f162173dc1</em> to <em>af8b427e4baa8dab4d61e9f5e94a121b2ecac389</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=af8b427e4baa8dab4d61e9f5e94a121b2ecac389"><span class="icon"></span>af8b427</a></td><td><code>py3: fix a couple AttributeErrors on dict.iteritems</code>
</td></tr></table>
TicketErik BrayFri, 27 Jul 2018 14:31:06 GMT
https://trac.sagemath.org/ticket/25948#comment:6
https://trac.sagemath.org/ticket/25948#comment:6
<p>
We've had this discussion before, and IMO unless you can show that it has a noticeable performance impact in this specific case I don't care enough to change it.
</p>
TicketTravis ScrimshawFri, 27 Jul 2018 20:16:12 GMTcommit, branch changed
https://trac.sagemath.org/ticket/25948#comment:7
https://trac.sagemath.org/ticket/25948#comment:7
<ul>
<li><strong>commit</strong>
changed from <em>af8b427e4baa8dab4d61e9f5e94a121b2ecac389</em> to <em>91503870b530276d48bc43b0589abe0f8593f834</em>
</li>
<li><strong>branch</strong>
changed from <em>u/embray/python3/misc/dict-iterators</em> to <em>u/tscrim/python3_misc_dict_iters-25948</em>
</li>
</ul>
<p>
I care a little bit more. :) If my change is good, then positive review.
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=91503870b530276d48bc43b0589abe0f8593f834"><span class="icon"></span>9150387</a></td><td><code>Remove itervalues.</code>
</td></tr></table>
TicketErik BrayMon, 30 Jul 2018 16:49:29 GMT
https://trac.sagemath.org/ticket/25948#comment:8
https://trac.sagemath.org/ticket/25948#comment:8
<p>
It looks like you changed it in some places but not others. Was that a result of more specific benchmarks?
</p>
<p>
The only reason I don't care as that the benchmark you cite could be very significant in some contexts, or very insignificant or even wrong in other contexts. I certainly care about about more specific benchmarks.
</p>
TicketgitMon, 30 Jul 2018 21:53:28 GMTcommit changed
https://trac.sagemath.org/ticket/25948#comment:9
https://trac.sagemath.org/ticket/25948#comment:9
<ul>
<li><strong>commit</strong>
changed from <em>91503870b530276d48bc43b0589abe0f8593f834</em> to <em>69f8e03fa62eb8ed4ddc0c113486d0aab7553a12</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=64b21f1201280eef352e501b6d9f99cfe9d60059"><span class="icon"></span>64b21f1</a></td><td><code>Merge branch 'u/tscrim/python3_misc_dict_iters-25948' of git://trac.sagemath.org/sage into u/tscrim/python3_misc_dict_iters-25948</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=69f8e03fa62eb8ed4ddc0c113486d0aab7553a12"><span class="icon"></span>69f8e03</a></td><td><code>Making the output machine-independent.</code>
</td></tr></table>
TicketTravis ScrimshawMon, 30 Jul 2018 22:05:25 GMT
https://trac.sagemath.org/ticket/25948#comment:10
https://trac.sagemath.org/ticket/25948#comment:10
<p>
My branch
</p>
<pre class="wiki">sage: A = ClusterAlgebra(['A',[1,2],1])
sage: A.current_seed()
The initial seed of a Cluster Algebra with cluster variables x0, x1, x2 and no coefficients over Integer Ring
sage: for k in [0,1,2]*4:
....: A.current_seed().mutate(k)
....:
sage: %timeit A.F_polynomials_so_far()
The slowest run took 11.30 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 443 ns per loop
</pre><p>
vs your branch
</p>
<pre class="wiki">sage: %timeit A.F_polynomials_so_far()
The slowest run took 64.77 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 806 ns per loop
</pre><p>
So while this is not a big shift in timings (despite the 2x speedup), running this method repeatedly can start taking time:
</p>
<pre class="wiki">sage: A = ClusterAlgebra(['A',[1,2],1])
sage: %%timeit
....: A.reset_current_seed()
....: for k in [0,1,2]*6:
....: A.current_seed().mutate(k)
....: A.F_polynomials_so_far()
....:
The slowest run took 12.77 times longer than the fastest. This could mean that an intermediate result is being cached.
1000 loops, best of 3: 1.49 ms per loop
</pre><p>
vs your branch
</p>
<pre class="wiki">The slowest run took 12.22 times longer than the fastest. This could mean that an intermediate result is being cached.
1000 loops, best of 3: 1.58 ms per loop
</pre><hr />
<p>
I also changed some tests as they were failing on my machine and the patchbot to make them machine-independent (I am assuming they were passing on your machine).
</p>
TicketErik BrayTue, 31 Jul 2018 14:09:09 GMT
https://trac.sagemath.org/ticket/25948#comment:11
https://trac.sagemath.org/ticket/25948#comment:11
<blockquote class="citation">
<p>
I also changed some tests as they were failing on my machine and the patchbot to make them machine-independent (I am assuming they were passing on your machine).
</p>
</blockquote>
<p>
I'm not sure I understand this. Why would the polynomials be sorted differently on different machines? The sorting I had looks like it makes sense. If it was giving different results on Python 2 that still might be a real issue worth looking into instead of just doing key=str.
</p>
TicketErik BrayTue, 31 Jul 2018 14:11:26 GMTstatus changed
https://trac.sagemath.org/ticket/25948#comment:12
https://trac.sagemath.org/ticket/25948#comment:12
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
I see, here are a couple examples:
</p>
<pre class="wiki">sage -t --long src/sage/algebras/cluster_algebra.py
**********************************************************************
File "src/sage/algebras/cluster_algebra.py", line 101, in sage.algebras.cluster_algebra
Failed example:
sorted(A.cluster_variables_so_far())
Expected:
[x0, x1, (x0 + 1)/x1, (x1 + 1)/x0, (x0 + x1 + 1)/(x0*x1)]
Got:
[x1, (x0 + 1)/x1, x0, (x1 + 1)/x0, (x0 + x1 + 1)/(x0*x1)]
**********************************************************************
File "src/sage/algebras/cluster_algebra.py", line 1678, in sage.algebras.cluster_algebra.ClusterAlgebra.cluster_variables_so_far
Failed example:
sorted(A.cluster_variables_so_far())
Expected:
[x0, x1, (x1 + 1)/x0]
Got:
[x1, x0, (x1 + 1)/x0]
</pre><p>
I'm not sure what's up with that. Maybe there's another change on my Python 3 branch this is dependent on...
</p>
TicketErik BrayTue, 14 Aug 2018 14:08:46 GMTstatus changed
https://trac.sagemath.org/ticket/25948#comment:13
https://trac.sagemath.org/ticket/25948#comment:13
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
</ul>
<p>
I see now--<code>ClusterAlgebraElement</code> obtains its comparison operators from <code>ElementWrapper</code>, and defines things such that both <code><</code> and <code>></code> always return <code>False</code>. I'm not sure how I feel about that, but nevertheless it means from Python's perspective they can be "sorted", but the sorting is meaningless.
</p>
<p>
I'm not 100% sure I like <code>str</code> as a sort key in these examples, but it doesn't really matter either way, as long as it gives consistent results.
</p>
TicketVolker BraunFri, 17 Aug 2018 21:14:03 GMTstatus, branch changed; resolution set
https://trac.sagemath.org/ticket/25948#comment:14
https://trac.sagemath.org/ticket/25948#comment:14
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>branch</strong>
changed from <em>u/tscrim/python3_misc_dict_iters-25948</em> to <em>69f8e03fa62eb8ed4ddc0c113486d0aab7553a12</em>
</li>
</ul>
Ticket