Opened 5 months ago
Closed 3 months ago
#26276 closed defect (fixed)
py3: misc fixes for sage.modules
Reported by: | embray | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | sage-8.5 |
Component: | python3 | Keywords: | |
Cc: | Merged in: | ||
Authors: | Erik Bray | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | 7bf0631 (Commits) | Commit: | 7bf063141e92715384c824ab69988e0260176043 |
Dependencies: | Stopgaps: |
Description
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".
Change History (26)
comment:1 Changed 5 months ago by
- Status changed from new to needs_review
comment:2 Changed 5 months ago by
comment:3 Changed 5 months ago by
- Status changed from needs_review to needs_work
the branch is now red
comment:4 Changed 5 months ago by
- Commit changed from a6e4cad8f6d2c965033fb1fea85c4c6eed49df1d to 8766c3805a350202df10c872e7121aaf508ed840
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
|
comment:5 Changed 5 months ago by
- Status changed from needs_work to needs_review
comment:6 Changed 5 months ago by
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
).
comment:7 Changed 5 months ago by
It's not pointless. On Python 3 it's needed. The value returned by self._entries.iteritems()
is technically not an iterator strangely enough.
comment:8 Changed 5 months ago by
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.
comment:9 Changed 5 months ago by
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)
comment:10 Changed 5 months ago by
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?
comment:11 Changed 5 months ago by
Could we keep the changes in src/sage/modules/with_basis/morphism.py
for another ticket then ?
comment:12 Changed 5 months ago by
I would move it to another ticket if it is not necessary for Python3.
comment:13 follow-up: ↓ 14 Changed 5 months ago by
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.
comment:14 in reply to: ↑ 13 Changed 5 months ago by
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.
comment:15 Changed 5 months ago by
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.
comment:16 follow-up: ↓ 17 Changed 4 months ago by
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 ?
comment:17 in reply to: ↑ 16 Changed 4 months ago by
Replying to chapoton:
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).
comment:18 Changed 4 months ago by
- Milestone changed from sage-8.4 to sage-8.5
comment:19 Changed 4 months ago by
- Branch changed from u/embray/sage-modules/misc to public/ticket/26276
- Commit changed from 8766c3805a350202df10c872e7121aaf508ed840 to 26e8aa96a3164a729908e99027146587f69bf64f
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
|
comment:20 follow-up: ↓ 23 Changed 4 months ago by
- Reviewers set to Travis Scrimshaw
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.
comment:21 Changed 4 months ago by
- Commit changed from 26e8aa96a3164a729908e99027146587f69bf64f to 7bf063141e92715384c824ab69988e0260176043
Branch pushed to git repo; I updated commit sha1. New commits:
7bf0631 | trac 26276 minor detail
|
comment:22 Changed 4 months ago by
- Status changed from needs_review to positive_review
done, thanks. Setting to positive
comment:23 in reply to: ↑ 20 ; follow-up: ↓ 24 Changed 3 months ago by
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?
comment:24 in reply to: ↑ 23 Changed 3 months ago by
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.
comment:25 Changed 3 months ago by
I agree. It's redundant but let's just leave it.
comment:26 Changed 3 months ago by
- Branch changed from public/ticket/26276 to 7bf063141e92715384c824ab69988e0260176043
- Resolution set to fixed
- Status changed from positive_review to closed
With these fixes I have all of
./sage -t --long src/sage/modules/
passing.