Opened 3 years ago
Closed 3 years ago
#27121 closed enhancement (fixed)
Global function fields: divisors
Reported by:  klee  Owned by:  

Priority:  major  Milestone:  sage8.7 
Component:  algebra  Keywords:  
Cc:  Merged in:  
Authors:  Kwankyu Lee  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  642b659 (Commits, GitHub, GitLab)  Commit:  642b659d6da40abb604ec0b156bb2f4b5c4be87f 
Dependencies:  Stopgaps: 
Description
This is part of the metaticket #22982.
The goal of the present ticket is to add code for computing with divisors of global function fields.
Change History (14)
comment:1 Changed 3 years ago by
 Branch set to u/klee/27121
comment:2 Changed 3 years ago by
 Commit set to 86481c4b100914b4655e27fce0f387c24a5ec7d8
comment:3 Changed 3 years ago by
 Status changed from new to needs_review
comment:4 followups: ↓ 5 ↓ 7 Changed 3 years ago by
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 oneliner), 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:
.
comment:5 in reply to: ↑ 4 ; followup: ↓ 8 Changed 3 years ago by
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.
comment:6 Changed 3 years ago by
 Commit changed from 86481c4b100914b4655e27fce0f387c24a5ec7d8 to b9021897730be4f082d6645c33580c90f3447556
Branch pushed to git repo; I updated commit sha1. New commits:
b902189  Various fixes

comment:7 in reply to: ↑ 4 Changed 3 years ago by
Replying to tscrim:
Is there a way to minimize the duplication of
divisor
,divisor_of_zeros
, anddivisor_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 doDivisorGroup(field).zero()
and not be used to construct the0
of the divisor group. I am slightly in favor of removing it for the explicitness of the code (as it is a oneliner), 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 involveself
. 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 ofwhile len(s) > 0 and len(o) > 0:
.
Done.
comment:8 in reply to: ↑ 5 ; followup: ↓ 10 Changed 3 years ago by
Replying to klee:
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.
comment:9 Changed 3 years ago by
Also, the .. NOTE::
block should have two colons and the following text should be indented, but this might be moot.
comment:10 in reply to: ↑ 8 Changed 3 years ago by
Replying to tscrim:
Replying to klee:
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.
comment:11 Changed 3 years ago by
 Commit changed from b9021897730be4f082d6645c33580c90f3447556 to 642b659d6da40abb604ec0b156bb2f4b5c4be87f
Branch pushed to git repo; I updated commit sha1. New commits:
642b659  Fix docstring for __radd__

comment:12 followup: ↓ 13 Changed 3 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
Okay, if it does what you want, then positive review.
comment:13 in reply to: ↑ 12 Changed 3 years ago by
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!
comment:14 Changed 3 years ago by
 Branch changed from u/klee/27121 to 642b659d6da40abb604ec0b156bb2f4b5c4be87f
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Add divisors to function fields