Quotient of incompatible lattices

[orlitzky]

The quotient method of toric lattices accepts a sublattice and should check that the argument is in fact a sublattice. According to the documentation,

   INPUT:

   * "sub" -- sublattice of self;

   * "check" -- (default: True) whether or not to check that "sub"
     is a valid sublattice.

However, it is possible to take the quotient of a lattice N with a sublattice of M that is not compatible:

sage: K = Cone([(1,0,0),(0,1,0)])
sage: K.lattice()
3-d lattice N
sage: K.orthogonal_sublattice()
Sublattice <M(0, 0, 1)>
sage: K.lattice().quotient(K.orthogonal_sublattice())
2-d lattice, quotient of 3-d lattice N by Free module of degree 3 and rank 1 over Integer Ring
Echelon basis matrix:
[0 0 1]

This should raise an error instead.

CC: @novoselt

Component: geometry

Author: Andrey Novoseltsev

Branch/Commit: d762db7

Reviewer: Michael Orlitzky

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

[novoselt]

comment:1

Looking into it, what probably should raise an exception is

sage: K.lattice().submodule(K.orthogonal_sublattice())
Free module of degree 3 and rank 1 over Integer Ring
Echelon basis matrix:
[0 0 1]

while taking quotients of toric lattices by "generic" vectors and modules is OK.

[novoselt]

Branch: u/novoselt/quotient_of_incompatible_lattices

[novoselt]

Author: Andrey Novoseltsev

[novoselt]

Commit: 399bc73

[novoselt]

comment:3

Found a typo in doctests along the way!

---

New commits:

399bc73

Improve checks for toric lattice span and quotient.

[orlitzky]

comment:4

I added some tests/examples to the quotient method but noticed something strange when I went to do the same for the ToricLattice_quotient class. The code in the class calls submodule() which is where the error is expected to occur:

if check:
    try:
        W = V.submodule(W)
    except (TypeError, ArithmeticError):
        raise ArithmeticError("W must be a sublattice of V")

But the submodule() method isn't throwing one of those two errors -- it's throwing a ValueError. The result is that the user sees the exception thrown from submodule() and not the (nicer) error from the quotient class:

sage: from sage.geometry.toric_lattice import ToricLattice_quotient
sage: N = ToricLattice(3)
sage: M = ToricLattice(3, name='M')
sage: ToricLattice_quotient(N,M)
...
ValueError: M(1, 0, 0) can not generate a sublattice of 3-d lattice N

For the wishlist, it might help to report something other than the variable names in the ToricLattice_quotient error message. So instead of,

ArithmeticError: W must be a sublattice of V

it could be,

ArithmeticError: 3-d lattice M must be a sublattice of 3-d lattice N

but I realize that's hard to do if the user passes in weirder objects. We could get part of the way there by checking that V is in fact a lattice before doing the submodule() check. That way V will print nicely, and even if the user passes in something stupid it would read alright:

ArithmeticError: Petersen graph: Graph on 10 vertices is not a sublattice of 3-d lattice N

---

New commits:

399bc73	Improve checks for toric lattice span and quotient.
5f62fd8	Trac #19603: remove trailing whitespace (review).
3c1cb53	Trac #19603: add a few more examples and tests for lattice quotients (review).

[orlitzky]

Changed commit from 399bc73 to 3c1cb53

[orlitzky]

Reviewer: Michael Orlitzky

[orlitzky]

Changed branch from u/novoselt/quotient_of_incompatible_lattices to u/mjo/ticket/19603

[novoselt]

comment:5

In general it is a bit discouraged to include bad arguments into error messages because the code may be catching exceptions do deal with them, but formatting detailed error messages takes time (although we have some lazy strings, if I recall correctly).

These particular classes are derived from general free modules and then I tried to do the minimum possible changes to get custom output and handle different/dual lattices. It is probably a waste of effort to perfect one particular aspect of them instead of thinking of improving the overall design first ;-) So I am in favour of merging things as they are now (together with your commits) since it is an improvement.

[novoselt]

Changed branch from u/mjo/ticket/19603 to u/novoselt/ticket/19603

[novoselt]

comment:7

There was formatting error in my commit caught by the patchbot - hopefully everything is OK now.

---

New commits:

5858c68

Typo in doc formatting.

[novoselt]

Changed commit from 3c1cb53 to 5858c68

[orlitzky]

Changed branch from u/novoselt/ticket/19603 to u/mjo/ticket/19603

[orlitzky]

Changed commit from 5858c68 to d762db7

[orlitzky]

comment:8

That's OK, I didn't have my hopes up for the error message.

My other concern was basically that this branch looked unreachable,

try:
    W = V.submodule(W)
except (TypeError, ArithmeticError):
    raise ArithmeticError("W must be a sublattice of V")

because the error that gets thrown in submodule() is a ValueError. But I see that I was wrong, and I added the doctest once I figured out how to cause that ArithmeticError.

Positive review otherwise.

---

New commits:

399bc73	Improve checks for toric lattice span and quotient.
5f62fd8	Trac #19603: remove trailing whitespace (review).
3c1cb53	Trac #19603: add a few more examples and tests for lattice quotients (review).
5858c68	Typo in doc formatting.
d762db7	Trac #19603: test the error branch of ToricLattice_quotient (review).

[novoselt]

comment:9

That branch is probably due to copy-pasting from general span modules. Thanks for the review!

[vbraun]

Changed branch from u/mjo/ticket/19603 to d762db7