Opened 3 years ago

Closed 3 years ago

#27558 closed enhancement (fixed)

Global function fields: last fixes and additions

Reported by: klee Owned by:
Priority: minor Milestone: sage-8.8
Component: algebra Keywords:
Cc: Merged in:
Authors: Kwankyu Lee Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: a9b9d3a (Commits, GitHub, GitLab) Commit: a9b9d3a1c13b65b5312f80aa181fb84fcd28b799
Dependencies: Stopgaps:

Status badges

Description

This is part of the meta-ticket #22982.

This ticket is for last fixes and additions to the global function field code.

Change History (25)

comment:1 Changed 3 years ago by klee

  • Authors set to Kwankyu Lee
  • Branch set to u/klee/27558

comment:2 Changed 3 years ago by git

  • Commit set to c109c6e6ba7d3b364d9619c890fbabff0eef3da0

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

c109c6eLast fixes and additions

comment:3 Changed 3 years ago by git

  • Commit changed from c109c6e6ba7d3b364d9619c890fbabff0eef3da0 to 4e2cae96205e5b434133f3bd832adcf8e7487bec

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

4e2cae9A typo

comment:4 Changed 3 years ago by klee

  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to enhancement

comment:5 Changed 3 years ago by git

  • Commit changed from 4e2cae96205e5b434133f3bd832adcf8e7487bec to 7511700bd0359faf12418791e0640574ee1d5e75

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7511700Last fixes and additions

comment:6 Changed 3 years ago by klee

  • Dependencies set to #27347

comment:7 Changed 3 years ago by git

  • Commit changed from 7511700bd0359faf12418791e0640574ee1d5e75 to 6b18a929dcbfd36e41633dca5fa0d6a102bcf3e0

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

6b18a92Last fixes and additions

comment:8 Changed 3 years ago by git

  • Commit changed from 6b18a929dcbfd36e41633dca5fa0d6a102bcf3e0 to 12890d96b82853f167104f3a6ca3e5cfaa4d3a7f

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

12890d9Last fixes and additions

comment:9 Changed 3 years ago by klee

  • Status changed from needs_review to needs_work

comment:10 Changed 3 years ago by git

  • Commit changed from 12890d96b82853f167104f3a6ca3e5cfaa4d3a7f to 21b3121bfb58a9fd20f5f8bc963a3369f70c2745

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

21b3121Last fixes and additions

comment:11 Changed 3 years ago by git

  • Commit changed from 21b3121bfb58a9fd20f5f8bc963a3369f70c2745 to 4d17d0dc94d48ac32f861def4e61781710458ec5

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

4d17d0dFix a typo

comment:12 Changed 3 years ago by git

  • Commit changed from 4d17d0dc94d48ac32f861def4e61781710458ec5 to 0f6e310f5dc122b11441b58143599cebe3c5e105

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

3bf2101Add scalar multiplication operator
4bf0f2fAdd coercion from Laurent polynomials
86ccdc4Add polynomial method and coercions from Laurent polynomial ring
e028ed6A little change in truncate method
8f8adf2Add __getitem__ special method to series
b3a6792Add division operator
575022cTypos
c55925fAdded a recursive definition example
410ca38Add prec and approximate_series method
0f6e310Merged with #27347

comment:13 Changed 3 years ago by git

  • Commit changed from 0f6e310f5dc122b11441b58143599cebe3c5e105 to 0c3333b19e3376a237a65feab9bcfc101edc1227

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

0c3333bLast fixes and additions

comment:14 Changed 3 years ago by klee

  • Dependencies #27347 deleted

comment:15 Changed 3 years ago by klee

  • Status changed from needs_work to needs_review

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

A few near trivial comments:

IIRC, it is considered better practice to do this:

-        return self._f.valuation(place) + 2*min(F(x).valuation(place), 0) \
-                + F.different().valuation(place)
+        return (self._f.valuation(place) + 2*min(F(x).valuation(place), 0)
+                + F.different().valuation(place))

so if something is unmatched, it is guaranteed to yield an error.

The list is unnecessary in here: return hash(tuple(sorted(list(self._data)))).

`ValueError` -> ``ValueError``

Instead of importing IntegerRing and asking for an instance, I would just import ZZ and use that (IMO, it also makes the code easier to read).

e = f * sep **(-val) -> e = f * sep**(-val) (PEP8 allows higher priority operators to not have a space around them; otherwise you need the space on the other side ;))

comment:17 Changed 3 years ago by git

  • Commit changed from 0c3333b19e3376a237a65feab9bcfc101edc1227 to 651c5422b3bae9b1831ebebcbfdbebb50ebb4f83

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

651c542Fixes for reviewer comments

comment:18 in reply to: ↑ 16 Changed 3 years ago by klee

Replying to tscrim:

A few near trivial comments:

Each of your comments is valuable :-)

IIRC, it is considered better practice to do this:

-        return self._f.valuation(place) + 2*min(F(x).valuation(place), 0) \
-                + F.different().valuation(place)
+        return (self._f.valuation(place) + 2*min(F(x).valuation(place), 0)
+                + F.different().valuation(place))

so if something is unmatched, it is guaranteed to yield an error.

I agree. Fixed.

The list is unnecessary in here: return hash(tuple(sorted(list(self._data)))).

Actually there was a bug here. It is now hash(tuple(sorted(self._data.items())))

`ValueError` -> ``ValueError``

Done.

Instead of importing IntegerRing and asking for an instance, I would just import ZZ and use that (IMO, it also makes the code easier to read).

Right. Done.

e = f * sep **(-val) -> e = f * sep**(-val) (PEP8 allows higher priority operators to not have a space around them; otherwise you need the space on the other side ;))

Right. It is a typo. Fixed.

comment:19 Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Thank you. LGTM.

comment:20 Changed 3 years ago by git

  • Commit changed from 651c5422b3bae9b1831ebebcbfdbebb50ebb4f83 to 7d2b56aa83aed1e3791279d7f3f65afbe4324541
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

7d2b56aA tiny fix in the last minute

comment:21 Changed 3 years ago by klee

Travis, I put in a very small fix into the already positively-reviewed patches. This fixes a bug I found only lately. With the fix, this works:

sage: K.<x> = FunctionField(GF(5)); _.<Y> = K[]
sage: L.<y> = K.extension(Y-1)
sage: L.genus()
0

Previously, this failed basically because L is a trivial extension of K.

I am sorry to add this in the last minute. If you are ok, I would set it positive review again.

comment:22 Changed 3 years ago by tscrim

Once you add a doctest showing this, you can set it back to a positive review.

comment:23 Changed 3 years ago by git

  • Commit changed from 7d2b56aa83aed1e3791279d7f3f65afbe4324541 to a9b9d3a1c13b65b5312f80aa181fb84fcd28b799

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

a9b9d3aAdd a doctest to test a trivial extension

comment:24 Changed 3 years ago by klee

  • Status changed from needs_review to positive_review

comment:25 Changed 3 years ago by vbraun

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