ToricRationalDivisorClass: Integer/Rational mixed up

[jdemeyer]

In src/sage/schemes/toric/divisor.py, there is:

class ToricRationalDivisorClassGroup_basis_lattice(FreeModule_ambient_pid):
    def __init__(self, group):
        # ...
        super(ToricRationalDivisorClassGroup_basis_lattice, self).__init__(
            ZZ, group.dimension())

(note the ZZ)

However, in src/sage/schemes/toric/divisor_class.pyx, there is

cdef class ToricRationalDivisorClass(Vector_rational_dense):

So, do we want rationals or integers? This causes segfaults when improving vectors in #17562.

CC: @vbraun @novoselt

Component: geometry

Branch/Commit: u/jdemeyer/ticket/17570 @ b2c68ca

Reviewer: Jeroen Demeyer

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

[vbraun]

comment:1

It should be QQ (the "rational" part). I vaguely remember that this caused some problem with the old modules. If you can change it then please do.

[jdemeyer]

comment:2

The problem is when you're constructing a Cone using this as lattice (the Kaehler_cone or Mori_cone), it complains since it wants integer lattices.

[jdemeyer]

Branch: u/jdemeyer/ticket/17570

[jdemeyer]

Commit: b2c68ca

[jdemeyer]

Author: Jeroen Demeyer

[jdemeyer]

comment:4

This branch passes all doctests, however I admit that I do not understand what this code is doing and whether my fix does The Right Thing.

---

New commits:

b2c68ca

ToricRationalDivisorClassGroup_basis_lattice should be defined over QQ

[novoselt]

comment:5

Replying to @jdemeyer:

This branch passes all doctests, however I admit that I do not understand what this code is doing and whether my fix does The Right Thing.

It is most definitely not the right thing to do in the cone module - a lattice is a free module over integers and not rationals! (On the other hand there is nothing wrong with a vector over rationals related to a cone defined over integers.) I can try to figure out what is the right thing to do here, but not this minute.

[jdemeyer]

comment:6

So, what you really want is a free ZZ-module generated by vectors which might have coefficients in QQ, is that right?

That's actually legal:

sage: M = (ZZ^2).span([(1,1/3),(0,1/2)])
sage: M
Free module of degree 2 and rank 2 over Integer Ring
Echelon basis matrix:
[  1 1/3]
[  0 1/2]
sage: M.base_ring()
Integer Ring
sage: type(M.basis()[0])
<type 'sage.modules.vector_rational_dense.Vector_rational_dense'>

So the problem might be solvable by deriving from

sage: type(M)
<class 'sage.modules.free_module.FreeModule_submodule_pid_with_category'>

instead of FreeModule_generic_pid.

[jdemeyer]

comment:7

I'm wondering if we should introduce a new method coefficient_ring or something on the element class which would return QQ in this case.

[jdemeyer]

comment:8

The more I'm thinking about this, the more I'm thinking that this is not a bug.

[jdemeyer]

comment:9

This is really a whole can of worms and this toric stuff is just the only place where it is exposed by doctests.

A lot of code in free modules assumes that the base_ring() of the parent (i.e. the free module) is the same ring as the ring of coefficients of the elements.

[jdemeyer]

Changed author from Jeroen Demeyer to none

[jdemeyer]

Reviewer: Jeroen Demeyer

[jdemeyer]

comment:11

This is not a bug, see https://groups.google.com/forum/#!topic/sage-devel/7n5xtT9Dw2g