Move the last few module classes from Module_old to Module

[pjbruin]

After #10513, only the following module classes still inherit from Module_old, which is being deprecated:

1. sage.modular.abvar.finite_subgroup.FiniteSubgroup
2. sage.modular.overconvergent.genus0.OverconvergentModularFormsSpace
3. sage.structure.formal_sum.FormalSums
   The goal of this ticket is to make these classes inherit from Module instead, to make them use the new coercion model, and to add a deprecation warning to Module_old.

Depends on #10513

CC: @jpflori @simon-king-jena

Component: commutative algebra

Keywords: module coercion

Author: Peter Bruin

Branch/Commit: c3356eb

Reviewer: Vincent Delecroix, Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/17543

[pjbruin]

Dependencies: #10513

[pjbruin]

Branch: u/pbruin/17543-Module_old

[pjbruin]

Author: Peter Bruin

[pjbruin]

Commit: d6b5edb

[pjbruin]

Changed keywords from module to module coercion

[videlec]

comment:3

Many conflicts on the last beta that I was not able to deal with completely. Moreover, I guess that it would also be safer to rebase over #17585 and #17850 that should be in sage-6.6.beta2. I am willing to review it if you find time to do the rebase after sage-6.6.beta2 is out.

Vincent

[pjbruin]

comment:4

Replying to @videlec:

Many conflicts on the last beta that I was not able to deal with completely. Moreover, I guess that it would also be safer to rebase over #17585 and #17850 that should be in sage-6.6.beta2. I am willing to review it if you find time to do the rebase after sage-6.6.beta2 is out.

I think it is best to wait until #10513 is merged.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

b1287e7	Merge branch 'ticket/17561' into ticket/10513
f0957f0	Trac 10513: use FreeModules(base_ring.category()) instead of FreeModules(base_ring)
6b63026	Trac 10513: method FreeModule_generic.list() to replace FiniteEnumeratedSets magic
6d5aaa1	Merge branch 'develop' into ticket/10513-coercion_and_categories_for_modules
d1db7ea	Trac 10513: construct coerce_map as default conversion map instead of matrix morphism
7b09742	Trac 10513: checking if element_class is a method is no longer necessary
ac685ec	Trac 10513: update doctests
ac70986	Trac 10513: small documentation improvement
dd93dea	Merge branch 'ticket/10513-coercion_and_categories_for_modules' into ticket/17543-Module_old
7f8a5de	Trac 17543: remove unnecessary check

[sagetrac-git]

Changed commit from d6b5edb to 7f8a5de

[pjbruin]

comment:6

After resolving the merge conflicts for #10513 there was only one left on this ticket.

[videlec]

comment:7

Hi Peter,

I do only have very minor points.

This error message is not very nice

"x does not define an element of self"

I would rather be more explicit

"x (={}) does not define an element of {}".format(x, self)

(I notice that you are not responsible for that)

Why did you create the method generator_orders? Why not instead replace the code in abelian_iterator?

diff --git a/src/sage/structure/gens_py.py b/src/sage/structure/gens_py.py
index c272503..e22a8a2 100644
--- a/src/sage/structure/gens_py.py
+++ b/src/sage/structure/gens_py.py
@@ -52,7 +52,7 @@ def abelian_iterator(M):
         yield M(0)
         return
 
-    stop = list(M.generator_orders())
+    stop = [g.additive_order() for g in G]
     for i in range(len(stop)):
         if stop[i] is infinity:
             raise ArithmeticError("%s is not finite."%M)

not for this ticket

Do you know why FormalSums is in sage.structure? It makes no sense to me. It should rather be in sage.modules. Moreover it intersects very much with the combinatorial free modules

sage: C = CombinatorialFreeModule(ZZ,GF(7))
sage: C.an_element()
2*B[0] + 2*B[1] + 3*B[2]

What do you think?

what gens_py.py is doing in sage.structure?! I would rather put it in sage.combinat or sage.misc.

[jdemeyer]

comment:9

Replying to @videlec:

"x (={}) does not define an element of {}".format(x, self)

Even better: "{!r} does not define an element of {}".format(x, self)

[sagetrac-git]

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

256df27

Trac 17543: reviewer comments

[sagetrac-git]

Changed commit from 7f8a5de to 256df27

[pjbruin]

comment:11

Replying to @videlec:

This error message is not very nice

"x does not define an element of self"

I changed to "cannot convert {!r} into {}".format(x, self) (also because it sounds nicer: perhaps the user passes an object that makes mathematical sense but for which conversion does not work for some reason).

Why did you create the method generator_orders? Why not instead replace the code in abelian_iterator?

Good idea; I changed this as you suggested. Maybe generator_orders() can be regarded as belonging to the obsolete ParentWithAdditiveAbelianGens.

not for this ticket

Do you know why FormalSums is in sage.structure? It makes no sense to me. It should rather be in sage.modules.

Yes, it would certainly makes more sense for it to be there.

what gens_py.py is doing in sage.structure?! I would rather put it in sage.combinat or sage.misc.

Probably because it is only used in sage.structure.parent_gens...

[nbruin]

comment:13

Replying to @pjbruin:

I changed to "cannot convert {!r} into {}".format(x, self) (also because it sounds nicer: perhaps the user passes an object that makes mathematical sense but for which conversion does not work for some reason).

Caveat: In Python, exception paths can end up being pretty common. For instance, the coercion framework will frequently use "leap before you look", relying on catching the exception if that happens. With the line above you can end up with expensive strings processing in a situation where the string never gets displayed. I haven't checked whether that's actually the case for this string and it's a reason to be conservative with constructing complicated error strings. An alternative (if it's necessary) is to use "lazy" strings, where the string reps of the constituents are only asked if the "lazy string" itself is asked for its representation.

(mind you, our current object for this, LazyFormat, is only faster when used with rather complicated parameters, but if we ever find that error message construction is a bottleneck, we could probably optimize LazyFormat to really become competitive)

[videlec]

comment:14

Two patchbots are not happy but for no serious reason. The strangely failing doctest on eddy is perfectly fine on my computer.

Good to go!

Vincent

[videlec]

Reviewer: Vincent Delecroix

[videlec]

comment:15

Replying to @nbruin:

Replying to @pjbruin:

I changed to "cannot convert {!r} into {}".format(x, self) (also because it sounds nicer: perhaps the user passes an object that makes mathematical sense but for which conversion does not work for some reason).

Caveat: In Python, exception paths can end up being pretty common. For instance, the coercion framework will frequently use "leap before you look", relying on catching the exception if that happens. With the line above you can end up with expensive strings processing in a situation where the string never gets displayed. I haven't checked whether that's actually the case for this string and it's a reason to be conservative with constructing complicated error strings. An alternative (if it's necessary) is to use "lazy" strings, where the string reps of the constituents are only asked if the "lazy string" itself is asked for its representation.

(mind you, our current object for this, LazyFormat, is only faster when used with rather complicated parameters, but if we ever find that error message construction is a bottleneck, we could probably optimize LazyFormat to really become competitive)

Would be cool that string formatting to be fast and uniformized in errors. Why Python does not have a native solution for that?! Right now, I do prefer clear error messages instead of potentially faster coercion discovery.

Vincent

[nbruin]

comment:16

Replying to @videlec:

Would be cool that string formatting to be fast and uniformized in errors. Why Python does not have a native solution for that?! Right now, I do prefer clear error messages instead of potentially faster coercion discovery.

I was wrong in my timing. The difference is considerable even here. Taking one of the examples from the documentation:

sage: from sage.misc.lazy_format import LazyFormat
sage: J = J0(23)
sage: G = J.torsion_subgroup(11)
sage: G(J.torsion_subgroup(3).0)
sage: self=G
sage: x=J.torsion_subgroup(3).0
sage: %timeit "cannot convert {!r} into {}".format(x, self)
10000 loops, best of 3: 38.4 µs per loop
sage: %timeit "TypeError: cannot convert %s into %s"%(x,self)
10000 loops, best of 3: 33.2 µs per loop
sage: %timeit LazyFormat("TypeError: cannot convert %s into %s")%(x,self)
100000 loops, best of 3: 1.74 µs per loop

I think you should consider putting LazyFormat in the error messages (it's basically a drop-in replacement) or argue that the relevant error messages are only constructed with really cheap arguments in time-critical situations, or that these error messages never get raised in time-critical situations.

[simon-king-jena]

comment:17

Note that a few years ago I introduced a lazy attribute error to be used in sage.structure..., that had a surprising impact on the performance of "real" computations. One of the tricks was to have precisely one instance of the lazy error message; so, one doesn't even lose time to create a new instance for each error being raised (it is only needed to set some cdef attribute of the lazy error message object).

We did some experiments whether lazy strings could achieve the same, but they couldn't compete. However, what we did there was restricted to attribute errors.

In any case, I agree that lazy error messages would be an improvment in the current situation.

[nbruin]

comment:18

Replying to @simon-king-jena:

We did some experiments whether lazy strings could achieve the same, but they couldn't compete. However, what we did there was restricted to attribute errors.

I'd imagine that's faster, but it also has different semantics: if you have a traceback in which several such error messages occur, you'd have inaccurate information. The LazyFormat on the other hand is basically a drop-in replacement and, if used properly (i.e., fresh object every time rather than copying) it's probably quite acceptable (see #14585 for improvements though).

[pjbruin]

comment:19

I ran all doctests in sage.modular after doing

--- a/src/sage/modular/abvar/finite_subgroup.py
+++ b/src/sage/modular/abvar/finite_subgroup.py
@@ -651,6 +651,7 @@ class FiniteSubgroup(Module):
             z = self.abelian_variety().vector_space()(x, check=check)
             if z in self.lattice():
                 return self.element_class(self, z, check=False)
+        print('raising complicated error')
         raise TypeError("cannot convert {!r} into {}".format(x, self))
 
 

This shows that the error is only thrown in the doctest of _element_constructor_() (this method) and in __contains__(), which is implemented in a non-standard way. Hence I do not suspect string formatting is something we should worry about too much in this case.

[sagetrac-git]

Changed commit from 256df27 to ddbf101

[sagetrac-git]

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

ddbf101

Trac 17543: simplify FiniteSubgroup._element_constructor_()

[pjbruin]

comment:21

The latest commit simplifies FiniteSubgroup._element_constructor_(). The new version does not generate a formatted error message itself; the step of converting x into the lattice may still do this, as one of the doctests shows.

[tscrim]

Changed commit from ddbf101 to c3356eb

[tscrim]

comment:22

I'm okay with your latest round of changes. I moved some of the documentation around and added some extra (long) doctests. I'd be happy setting this to positive review as it currently stands (up to someone checking my changes).

---

New commits:

4e78e98	Merge branch 'u/pbruin/17543-Module_old' of trac.sagemath.org:sage into u/tscrim/module_old-17543
c3356eb	Added some doctests.

[tscrim]

Changed reviewer from Vincent Delecroix to Vincent Delecroix, Travis Scrimshaw

[tscrim]

Changed branch from u/pbruin/17543-Module_old to u/tscrim/module_old-17543

[pjbruin]

comment:23

Replying to @tscrim:

I'm okay with your latest round of changes. I moved some of the documentation around and added some extra (long) doctests. I'd be happy setting this to positive review as it currently stands (up to someone checking my changes).

OK, thanks!

[vbraun]

Changed branch from u/tscrim/module_old-17543 to c3356eb