Opened 3 years ago

Closed 3 years ago

#27121 closed enhancement (fixed)

Global function fields: divisors

Reported by: klee Owned by:
Priority: major Milestone: sage-8.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:

Status badges

Description

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.

Change History (14)

comment:1 Changed 3 years ago by klee

  • Branch set to u/klee/27121

comment:2 Changed 3 years ago by git

  • Commit set to 86481c4b100914b4655e27fce0f387c24a5ec7d8

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

86481c4Add divisors to function fields

comment:3 Changed 3 years ago by klee

  • Authors set to Kwankyu Lee
  • Status changed from new to needs_review

comment:4 follow-ups: Changed 3 years ago by tscrim

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:.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 3 years ago by 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?

        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 git

  • Commit changed from 86481c4b100914b4655e27fce0f387c24a5ec7d8 to b9021897730be4f082d6645c33580c90f3447556

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

b902189Various fixes

comment:7 in reply to: ↑ 4 Changed 3 years ago by klee

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.

comment:8 in reply to: ↑ 5 ; follow-up: Changed 3 years ago by 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.

comment:9 Changed 3 years ago by tscrim

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 klee

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 git

  • Commit changed from b9021897730be4f082d6645c33580c90f3447556 to 642b659d6da40abb604ec0b156bb2f4b5c4be87f

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

642b659Fix docstring for __radd__

comment:12 follow-up: Changed 3 years ago by tscrim

  • 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 klee

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 vbraun

  • Branch changed from u/klee/27121 to 642b659d6da40abb604ec0b156bb2f4b5c4be87f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.