Global function fields: divisors

[kwankyu]

This is part of the meta-ticket #22982.

The goal of the present ticket is to add code for computing with divisors of global function fields.

Component: algebra

Author: Kwankyu Lee

Branch/Commit: 642b659

Reviewer: Travis Scrimshaw

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

[kwankyu]

Branch: u/klee/27121

[sagetrac-git]

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

86481c4

Add divisors to function fields

[sagetrac-git]

Commit: 86481c4

[kwankyu]

Author: Kwankyu Lee

[tscrim]

comment:4

Here are my comments.

The __radd__ should not be necessary as the coercion framework should take care of that (which you are using with _add_), unless you are trying to explicitly avoid the coercion framework (as the coercions you would need are not (or should not) be there).

Is there a way to minimize the duplication of divisor, divisor_of_zeros, and divisor_of_poles in the two files (say, by putting them in a common base class for the respective files)? It all looks like basically the same code.

self.divisor_group()(0) -> self.divisor_group().zero() (the latter is cached).

Similarly, I am not sure I like having the zero_divisor method. If anything, it should do DivisorGroup(field).zero() and not be used to construct the 0 of the divisor group. I am slightly in favor of removing it for the explicitness of the code (as it is a one-liner), but I can somewhat see how it might be a more natural operation.

I don't see the point in the valuation_ring method as it does not involve self. This was something I missed on a previous review, but I noticed here because of the diff.

The keys here is unnecessary: sorted(self._data.keys()).

It is faster to use while s and o: instead of while len(s) > 0 and len(o) > 0:.

[kwankyu]

comment:5

Replying to @tscrim:

The __radd__ should not be necessary as the coercion framework should take care of that (which you are using with _add_), unless you are trying to explicitly avoid the coercion framework (as the coercions you would need are not (or should not) be there).

How could I achieve the following without __radd__? An integer (in particular zero) is not supposed to be coerced to a place. Then how the coercion framework could enable the example below. Would you give me hints?

        This is only to support the ``sum`` function, that adds
        the argument to initial (int) zero.

        EXAMPLES::

            sage: k.<a>=GF(2)
            sage: K.<x>=FunctionField(k)
            sage: sum(K.places_finite())
            Place (x) + Place (x + 1)

        .. NOTE:

        This does not work though::

            sage: 0 + K.place_infinite()
            Traceback (most recent call last):
            ...
            TypeError: unsupported operand parent(s) for +: ...

        The reason is that the ``0`` is a Sage integer, for which
        the coercion system applies. 

[sagetrac-git]

Changed commit from 86481c4 to b902189

[sagetrac-git]

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

b902189

Various fixes

[kwankyu]

comment:7

Replying to @tscrim:

Is there a way to minimize the duplication of divisor, divisor_of_zeros, and divisor_of_poles in the two files (say, by putting them in a common base class for the respective files)? It all looks like basically the same code.

Right. Done.

self.divisor_group()(0) -> self.divisor_group().zero() (the latter is cached).

Done.

Similarly, I am not sure I like having the zero_divisor method. If anything, it should do DivisorGroup(field).zero() and not be used to construct the 0 of the divisor group. I am slightly in favor of removing it for the explicitness of the code (as it is a one-liner), but I can somewhat see how it might be a more natural operation.

Done.

I don't see the point in the valuation_ring method as it does not involve self. This was something I missed on a previous review, but I noticed here because of the diff.

Removed.

The keys here is unnecessary: sorted(self._data.keys()).

Done.

It is faster to use while s and o: instead of while len(s) > 0 and len(o) > 0:.

Done.

[tscrim]

comment:8

Replying to @kwankyu:

Replying to @tscrim:

The __radd__ should not be necessary as the coercion framework should take care of that (which you are using with _add_), unless you are trying to explicitly avoid the coercion framework (as the coercions you would need are not (or should not) be there).

How could I achieve the following without __radd__? An integer (in particular zero) is not supposed to be coerced to a place. Then how the coercion framework could enable the example below. Would you give me hints?

To get this to work, you would need to allow 0 to convert into the parent (which is a special case within the coercion framework). So that is why this works:

sage: 0 + vector([1,2,1])
(1, 2, 1)
sage: coercion_model.canonical_coercion(0, v)
((0, 0, 0), (1, 2, 1))

So if you want it to work, you can hack something together to have 0 be some sort of allowed conversion to a dummy element.

[tscrim]

comment:9

Also, the .. NOTE:: block should have two colons and the following text should be indented, but this might be moot.

[kwankyu]

comment:10

Replying to @tscrim:

Replying to @kwankyu:

Replying to @tscrim:

The __radd__ should not be necessary as the coercion framework should take care of that (which you are using with _add_), unless you are trying to explicitly avoid the coercion framework (as the coercions you would need are not (or should not) be there).

How could I achieve the following without __radd__? An integer (in particular zero) is not supposed to be coerced to a place. Then how the coercion framework could enable the example below. Would you give me hints?

To get this to work, you would need to allow 0 to convert into the parent (which is a special case within the coercion framework). So that is why this works:

sage: 0 + vector([1,2,1])
(1, 2, 1)
sage: coercion_model.canonical_coercion(0, v)
((0, 0, 0), (1, 2, 1))

So if you want it to work, you can hack something together to have 0 be some sort of allowed conversion to a dummy element.

For vectors, 0 is naturally understood as zero vector, and the coercion system implements this. But as I said above, I don't want 0 to be coerced as some place (there is nothing like zero place). All I want is to make sum(a bunch of places) work.

[sagetrac-git]

Changed commit from b902189 to 642b659

[sagetrac-git]

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

642b659

Fix docstring for __radd__

[tscrim]

comment:12

Okay, if it does what you want, then positive review.

[tscrim]

Reviewer: Travis Scrimshaw

[kwankyu]

comment:13

Replying to @tscrim:

Okay, if it does what you want, then positive review.

I admit that the __radd__ thing is ugly. What I want is to allow 0 + p for a place p to result in the divisor p (=1*p; divisors are just formal sums of places). I wish that I could fix this later when I have a solution example of similar situation.

Thank you!

[vbraun]

Changed branch from u/klee/27121 to 642b659