Opened 10 months ago

Closed 9 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 10 months ago by embray

  • Status changed from new to needs_review

comment:2 Changed 10 months ago by embray

With these fixes I have all of ./sage -t --long src/sage/modules/ passing.

comment:3 Changed 10 months ago by chapoton

  • Status changed from needs_review to needs_work

the branch is now red

comment:4 Changed 10 months ago by git

  • Commit changed from a6e4cad8f6d2c965033fb1fea85c4c6eed49df1d to 8766c3805a350202df10c872e7121aaf508ed840

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

467602bpy3: sort the output from symmetrized_coordinate_sums so that it is deterministic on Python 2 and 3
137c1dfpy3: even this test class needs a __hash__ in order for the tests to work right
6e904c1py3: some items/iteritems fixes
0be1605py3: fix pickling of TriangularModuleMorphism
119245dpy3: fix pickling of ModuleMorphismFromMatrix
0c0f535py3: add a necessary int() for the arguments to islice
f553cfdfix what we agreed is likely a typo in this test
26dea60py3: replace testing exact hash value with something else
8766c38misc whitespace and other delinting

comment:5 Changed 10 months ago by embray

  • Status changed from needs_work to needs_review

comment:6 Changed 10 months ago by tscrim

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 10 months ago by embray

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 10 months ago by tscrim

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 10 months ago by embray

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 10 months ago by tscrim

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 10 months ago by chapoton

Could we keep the changes in src/sage/modules/with_basis/morphism.py for another ticket then ?

comment:12 Changed 10 months ago by tscrim

I would move it to another ticket if it is not necessary for Python3.

comment:13 follow-up: Changed 10 months ago by 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.

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 10 months ago by tscrim

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 10 months ago by embray

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: Changed 9 months ago by 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 ?

comment:17 in reply to: ↑ 16 Changed 9 months ago by tscrim

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 9 months ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

comment:19 Changed 9 months ago by chapoton

  • 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:

26e8aa9py3: sort the output from symmetrized_coordinate_sums so that it is deterministic on Python 2 and 3

comment:20 follow-up: Changed 9 months ago by tscrim

  • 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 9 months ago by git

  • Commit changed from 26e8aa96a3164a729908e99027146587f69bf64f to 7bf063141e92715384c824ab69988e0260176043

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

7bf0631trac 26276 minor detail

comment:22 Changed 9 months ago by chapoton

  • Status changed from needs_review to positive_review

done, thanks. Setting to positive

comment:23 in reply to: ↑ 20 ; follow-up: Changed 9 months ago by embray

Replying to tscrim:

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.

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 9 months ago by tscrim

Replying to embray:

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?

Because I missed it being defined. >_< Oh well, it doesn't really hurt anything by being there.

comment:25 Changed 9 months ago by embray

I agree. It's redundant but let's just leave it.

comment:26 Changed 9 months ago by vbraun

  • Branch changed from public/ticket/26276 to 7bf063141e92715384c824ab69988e0260176043
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.