Methods quotient, quotient_module are not the same for some modules from sage.modules

[mkoeppe]

The base class Module_free_ambient makes quotient and quotient_module the same, but some subclasses override only quotient.

sage: A = ZZ^3; V = A.span([[1,2,3], [4,5,6]])
sage: V.quotient
<bound method FreeModule_generic_pid.quotient of Free module of degree 3 and rank 2 over Integer Ring
Echelon basis matrix:
[1 2 3]
[0 3 6]>
sage: V.quotient_module
<bound method Module_free_ambient.quotient_module of Free module of degree 3 and rank 2 over Integer Ring
Echelon basis matrix:
[1 2 3]
[0 3 6]>

sage: A = QQ^3
sage: A.quotient
<bound method FreeModule_generic_field.quotient of Vector space of dimension 3 over Rational Field>
sage: A.quotient_module
<bound method Module_free_ambient.quotient_module of Vector space of dimension 3 over Rational Field>

CC: @tscrim @jhpalmieri @yyyyx4

Component: linear algebra

Author: Matthias Koeppe

Branch/Commit: cb82460

Reviewer: John Palmieri, Travis Scrimshaw

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

[jhpalmieri]

comment:1

Having multiple names for the same thing appears in other places, too: ZZ.quo?, ZZ.quotient?, and ZZ.quotient_ring? display different help strings although claiming to be synonyms for each other. Maybe not as big a deal since maybe not so many things inherit methods from ZZ, but still not ideal (heh).

[mkoeppe]

comment:2

I'm not concerned about the quo method (provided by Rings.ParentMethods and redundantly(?) by sage.rings.ring.Ring. It just delegates to quotient by calling the method.

[jhpalmieri]

comment:3

So in the case at hand, what's going on? Do we have

class Module_one(...):

    def quotient(...):

    quotient_module = quotient

whereas it would be better to have

class Module_one(...):

    def quotient(...):

    def quotient_module(...):
        return self.quotient(...)

Or would it be better to deprecate one or the other and focus on a single way of doing things? The Python philosophy is that there should ideally only be one obvious way to do things, so if we agree with that, then deprecation is the way to go.

[tscrim]

comment:4

Indeed, unfortunately there isn't a goo mechanism in Python to repeat the alias in subclasses other than having the redirect.

[mkoeppe]

comment:5

Replying to @jhpalmieri:

So in the case at hand, what's going on? Do we have

class Module_one(...):

    def quotient(...):

    quotient_module = quotient

whereas it would be better to have

class Module_one(...):

    def quotient(...):

    def quotient_module(...):
        return self.quotient(...)

Yes, exactly.

[mkoeppe]

Branch: u/mkoeppe/methods_quotient__quotient_module_are_not_the_same_for_some_modules_from_sage_modules

[mkoeppe]

Commit: ccc1f85

[mkoeppe]

comment:7

Having the alias quo was probably motivated by compatibility with Magma

---

New commits:

ccc1f85

src/sage/rings/ring.pyx: Remove methods duplicated from category

[mkoeppe]

comment:8

I can't comment on how important having the alias quo still is.

But there are probably good reasons to have both quotient and quotient_ring, for cases where also quotient_module makes sense

[sagetrac-git]

Changed commit from ccc1f85 to ef266af

[sagetrac-git]

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

ef266af

src/sage/rings/ring.pyx: Remove another method duplicated from category

[mkoeppe]

comment:10

Here's an incidental cleanup, removing duplications between base class and Rings.ParentMethods

[sagetrac-git]

Changed commit from ef266af to c446078

[sagetrac-git]

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

c446078

Modules.ParentMethods.quotient: New, delegates to .quotient_module; replaces alias Module_free_ambient.quotient

[mkoeppe]

Author: Matthias Koeppe

[jhpalmieri]

comment:14

Is there any guidance, or should there be, about which method to override, quotient or quotient_module?

[sagetrac-git]

Changed commit from c446078 to 4958d6d

[sagetrac-git]

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

4958d6d

Modules.ParentMethods.quotient: Expand docstring

[mkoeppe]

comment:16

How about this?

[jhpalmieri]

comment:17

I get a doctest failure in src/doc/en/thematic_tutorials/coercion_and_categories.rst. The changes look okay to me. Travis, any opinion?

[sagetrac-git]

Changed commit from 4958d6d to 80e2c38

[sagetrac-git]

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

80e2c38

src/doc/en/thematic_tutorials/coercion_and_categories.rst: Update doctest output

[tscrim]

comment:19

+1

I would say that redirecting to quotient_module is better for, say, algebras, as we could want to potentially keep the quotient-as-algebras and the quotient-as-modules different. The only other nag would be that most people will simply type (and override) the shorter-yet-still-clear quotient and they pay a small performance penalty of a redirect. (Of course, for the reason I just stated, this is much robust than the reverse because a programmer might implement the quotient as algebras with enforcement and expect quotient_module to still work and be surprised it doesn’t.) The redirect almost certainly shouldn’t have any significant impact on runtime.

[jhpalmieri]

comment:20

The doctest still fails: it is expecting __rtruediv__ and __truediv__ but not getting them.

[mkoeppe]

comment:21

Thanks - I was waiting for the build&test

[sagetrac-git]

Changed commit from 80e2c38 to cb82460

[sagetrac-git]

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

cb82460

src/doc/en/thematic_tutorials/coercion_and_categories.rst: Update doctest output (again)

[jhpalmieri]

Reviewer: John Palmieri, Travis Scrimshaw

[jhpalmieri]

comment:23

Positive review from me. I will flip the switch tomorrow if I don't hear otherwise.

[mkoeppe]

comment:25

Thanks!

[vbraun]

Changed branch from u/mkoeppe/methods_quotient__quotient_module_are_not_the_same_for_some_modules_from_sage_modules to cb82460