Closed embray closed 5 years ago
With these fixes I have all of ./sage -t --long src/sage/modules/
passing.
the branch is now red
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
467602b | py3: sort the output from symmetrized_coordinate_sums so that it is deterministic on Python 2 and 3 |
137c1df | py3: even this test class needs a `__hash__` in order for the tests to work right |
6e904c1 | py3: some items/iteritems fixes |
0be1605 | py3: fix pickling of TriangularModuleMorphism |
119245d | py3: fix pickling of ModuleMorphismFromMatrix |
0c0f535 | py3: add a necessary int() for the arguments to islice |
f553cfd | fix what we agreed is likely a typo in this test |
26dea60 | py3: replace testing exact hash value with something else |
8766c38 | misc whitespace and other delinting |
Two things from a very quick lookover:
The iter
here seems pointless as the result is an iterator:
return iter(self._entries.iteritems())
In Cython, you can still use iteritems
on a dict
in Python3 (from what Jeroen as said in the past). So this change should not be needed:
e = entries_dict
entries_dict = {}
try:
- for k, x in e.iteritems():
+ for k, x in e.items():
x = coefficient_ring(x)
if x:
entries_dict[k] = x
(unless e
needs a typecast to dict
).
It's not pointless. On Python 3 it's needed. The value returned by self._entries.iteritems()
is technically not an iterator strangely enough.
That is a bit strange, but okay, it will be what it has to be. I guess it shouldn't affect the timings of things. Although we probably want to leave a quick comment so someone less versed in Python3 doesn't have the same thought I had and remove the iter
.
I do think we should still revert the other change.
It's not that strange. On Python 3, dict.keys()
, dict.values()
, and dict.items()
return views:
>>> d = {}
>>> it = d.items()
>>> it
dict_items([])
>>> d['a'] = 1
>>> it
dict_items([('a', 1)])
>>> next(it)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: 'dict_items' object is not an iterator
>>> next(iter(it))
('a', 1)
I guess on Python3, Cython just considers iteritems
as a replacement for items
and gives the view.
Now feel free to call me paranoid, but I am a little worried about the extra indirection by defining the _on_basis
method. Since that forms the key part of the code and can be called very frequently, it could have some significant impact:
sage: from sage.modules.with_basis.morphism import ModuleMorphismFromMatrix
....: X = CombinatorialFreeModule(ZZ, [1,2]); X.rename("X"); x = X.basis()
....: Y = CombinatorialFreeModule(ZZ, [3,4]); Y.rename("Y"); y = Y.basis()
....: m = matrix([[1, 2], [3, 5]])
....: phi = ModuleMorphismFromMatrix(matrix=m, domain=X, codomain=Y, side="right")
....:
sage: %timeit phi(X.an_element())
The slowest run took 36.14 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 3.91 µs per loop
vs your branch
sage: %timeit phi(X.an_element())
The slowest run took 25.64 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 4.32 µs per loop
which is around a 10% slowdown. So I am not sure this is the best way forward as the pickling problem might be better addressed with a custom __reduce__
method. How much of those changes are necessary for Python3?
Could we keep the changes in src/sage/modules/with_basis/morphism.py
for another ticket then ?
I would move it to another ticket if it is not necessary for Python3.
I would suggest that if that code needs to perform on a level where even a method call is too much unacceptable overhead, that it be rewritten in Cython then.
I also considered a custom __reduce__
method, but I do feel that shoving a method of some object into an attribute of another object has a certain code smell to it.
Replying to @embray:
I would suggest that if that code needs to perform on a level where even a method call is too much unacceptable overhead, that it be rewritten in Cython then.
Yes, I think it should. These get used all over the place in things like the combinatorial Hopf algebra code. Although it may not have been done previously because of stuff with the category framework, multiple inheritance, concerns about increased debugging difficult, or just a previous lack of interest.
I also considered a custom
__reduce__
method, but I do feel that shoving a method of some object into an attribute of another object has a certain code smell to it.
I think that is a bad way to look at it. I would say it should be more like you are passing a function as input, which then gets called as part of the computation.
That's fair, though I probably might have instead explicitly made a lambda that wraps the dict in a closure and calls its getitem. But maybe that's a distinction without a difference (and that would still have a problem with pickling).
I'll take another look at it. I'm curious to see what happens as a first pass if I just redo some of that module in Cython.
Can we please start to move again here, and if necessary keep the changes in src/sage/modules/with_basis/morphism.py
for another ticket ?
Replying to @fchapoton:
Can we please start to move again here, and if necessary keep the changes in
src/sage/modules/with_basis/morphism.py
for another ticket ?
Yes, I think that would be good (and in that ticket, I might also Cythonize it too).
here is a branch without the changes in src/sage/modules/with_basis/morphism.py
The old branch is kept at u/embray/sage-modules/misc
New commits:
26e8aa9 | py3: sort the output from symmetrized_coordinate_sums so that it is deterministic on Python 2 and 3 |
Changed branch from u/embray/sage-modules/misc to public/ticket/26276
One last little thing:
- for k, x in e.iteritems():
+ for k, x in e.items():
Since e
is a dict
in a Cython file, I think it would be better to do
- for k, x in e.iteritems():
+ for k, x in (<dict> e).iteritems():
Once changed (or if you don't think it is a good idea), then you can set a positive review.
Reviewer: Travis Scrimshaw
Branch pushed to git repo; I updated commit sha1. New commits:
7bf0631 | trac 26276 minor detail |
done, thanks. Setting to positive
Replying to @tscrim:
Since
e
is adict
in a Cython file, I think it would be better to do- for k, x in e.iteritems(): + for k, x in (<dict> e).iteritems():
Once changed (or if you don't think it is a good idea), then you can set a positive review.
Why this? e
is already declared cdef dict
earlier in the method, so I'm not sure why you would want to add <dict> e
here?
Replying to @embray:
Why this?
e
is already declaredcdef dict
earlier in the method, so I'm not sure why you would want to add<dict> e
here?
Because I missed it being defined. >_<
Oh well, it doesn't really hurt anything by being there.
I agree. It's redundant but let's just leave it.
Changed branch from public/ticket/26276 to 7bf0631
Uploading all remaining
sage.modules
fixes from my Python 3 branch.This is a bit diverse but I think it's a small enough set of fixes to be grouped together. Also fixes some Python 2 bugs that were sort of left as "known failures".
Component: python3
Author: Erik Bray
Branch/Commit:
7bf0631
Reviewer: Travis Scrimshaw
Issue created by migration from https://trac.sagemath.org/ticket/26276