Opened 3 years ago

Closed 3 years ago

#26616 closed enhancement (fixed)

Global function fields: places

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: 8049919 (Commits, GitHub, GitLab) Commit: 80499197cf1facfc7757c824b2fd9055d10a2a49
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 places of global function fields.

Change History (21)

comment:1 Changed 3 years ago by klee

  • Branch set to u/klee/26616

comment:2 Changed 3 years ago by git

  • Commit set to 2531d4aa50da176f4ac33025151f8fc5168a0a2c

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

2531d4aAdd place

comment:3 Changed 3 years ago by git

  • Commit changed from 2531d4aa50da176f4ac33025151f8fc5168a0a2c to 2e6ebee5d9fa86c6b71f117f228c0388f277dce4

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

a5b1f66pyflakes fixes
2e6ebeeFixes for python3 compatibility

comment:4 Changed 3 years ago by klee

  • Status changed from new to needs_review

comment:5 Changed 3 years ago by git

  • Commit changed from 2e6ebee5d9fa86c6b71f117f228c0388f277dce4 to b0773fbe31efda1bbc93c35830ffce3e32279bf5

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

b0773fbRemove unused _cache

comment:6 Changed 3 years ago by git

  • Commit changed from b0773fbe31efda1bbc93c35830ffce3e32279bf5 to 1d8d0a6d45ddc5f6b8c93b52d5ddb5b3d82fe998

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

1d8d0a6Remove unnecessry @cahced_method

comment:7 Changed 3 years ago by klee

  • Authors set to Kwankyu Lee

comment:8 Changed 3 years ago by git

  • Commit changed from 1d8d0a6d45ddc5f6b8c93b52d5ddb5b3d82fe998 to ecc74051b0eb544787be617ffb26e2874d789122

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

ecc7405Remove unused import

comment:9 Changed 3 years ago by git

  • Commit changed from ecc74051b0eb544787be617ffb26e2874d789122 to b117c23c54b219af0309b1f12f913e9710f0ae8f

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

b117c23Merge branch 'develop'

comment:10 Changed 3 years ago by git

  • Commit changed from b117c23c54b219af0309b1f12f913e9710f0ae8f to b23cc8e796d75b8c00e497b92fb7214093f5b0a7

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

b23cc8eFix doctests that randomly fail

comment:11 Changed 3 years ago by git

  • Commit changed from b23cc8e796d75b8c00e497b92fb7214093f5b0a7 to d83ddd09106ddc38c242a92c6eb5436c4b09f352

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

d83ddd0Merge branch 'develop'

comment:12 Changed 3 years ago by git

  • Commit changed from d83ddd09106ddc38c242a92c6eb5436c4b09f352 to 69f866f20f0f4a444a1a1e6f91d7425cfbd06e86

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

69f866fMerge branch 'develop'

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

Some comments:

  • Perhaps it is my lack of understanding of the field, but is place common terminology for valuation()? It might be good to keep some indication about it being a prime element.
  • What is the reason for FunctionFieldElement_global.__cached_methods?
  • You should be able to replace [place for place in self._places_finite(degree)] with list(self._places_finite(degree)). Similarly for the infinite.
  • I think there is a grammar issue here: Return the unique place at infinite. Should it be Return the unique place at infinity.?
  • Instead of a @cached_method for _gens_over_base, I could consider making it a @lazy_attribute. (So you then use it as self._gens_over_base instead of self._gens_over_base().)
  • I am not sure about this change:
    -        norm = reduce(operator.mul, hnf.diagonal())
    +        norm = 1
    +        for e in hnf.diagonal():
    +            norm *= e
    
    as it might be slower. However, this is a minor thing (the new code is definitely more readable).
  • I do not think you need the lazy_import('sage.matrix.constructor', 'matrix') in sage/rings/function_field/valuation_ring.py.

Otherwise it does what it says it does, so LGTM.

comment:14 in reply to: ↑ 13 Changed 3 years ago by klee

Replying to tscrim:

Some comments:

  • Perhaps it is my lack of understanding of the field, but is place common terminology for valuation()? It might be good to keep some indication about it being a prime element.

"place" is established terminology in the function field theory. Places correspond, one-to-one, to (discrete) valuation rings of the function field, which defines valuation of the elements of the function field. In a later stage, I will introduce "divisors", which are central objects in the function field theory. A divisor is defined to be a formal sum of places. A place is not said to be prime, though "finite" places are in one-to-one correspondence with prime ideals of the "finite" maximal order.

  • What is the reason for FunctionFieldElement_global.__cached_methods?

I forgot :-) I removed it.

  • You should be able to replace [place for place in self._places_finite(degree)] with list(self._places_finite(degree)). Similarly for the infinite.

Done. Thanks.

  • I think there is a grammar issue here: Return the unique place at infinite. Should it be Return the unique place at infinity.?

Right. Fixed.

  • Instead of a @cached_method for _gens_over_base, I could consider making it a @lazy_attribute. (So you then use it as self._gens_over_base instead of self._gens_over_base().)

Ok. Done. Thanks for a good tip.

  • I am not sure about this change:
    -        norm = reduce(operator.mul, hnf.diagonal())
    +        norm = 1
    +        for e in hnf.diagonal():
    +            norm *= e
    
    as it might be slower. However, this is a minor thing (the new code is definitely more readable).

I guess Guido did not remove reduce in python3 for no reason. I did a simple experiment (with ints) in python3 and both take the same time. I would prefer to remove reduce.

  • I do not think you need the lazy_import('sage.matrix.constructor', 'matrix') in sage/rings/function_field/valuation_ring.py.

Removed.

Thank you for giving attention!!

comment:15 Changed 3 years ago by git

  • Commit changed from 69f866f20f0f4a444a1a1e6f91d7425cfbd06e86 to 8e86177f0b3187c3d0643e9f6565d3110d99be02

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

1409127Remove unused __cached_methods
bcc330fMerge branch 'develop' into function-fields-3-trac26616
8e86177Fixes for reviewer comments

comment:16 Changed 3 years ago by git

  • Commit changed from 8e86177f0b3187c3d0643e9f6565d3110d99be02 to 97d3bc384f9ff05e4a2cd881530998d897da291b

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

97d3bc3Add places to global function fields

comment:17 Changed 3 years ago by klee

Squashed and rebased to sage 8.6.

comment:18 Changed 3 years ago by tscrim

  • Milestone changed from sage-8.5 to sage-8.7
  • Reviewers set to Travis Scrimshaw

Thank you for the explanation about the definition of a place. I learned something new. ^_^

Do you think you could add that or some variant to place.py, either at the module-level or in some key class-level doc? Maybe also with a standard reference for the subject? Once that is done, positive review.

I forgot that reduce was moved to functools. Thanks for the reminder.

comment:19 Changed 3 years ago by git

  • Commit changed from 97d3bc384f9ff05e4a2cd881530998d897da291b to 80499197cf1facfc7757c824b2fd9055d10a2a49

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

8049919Add some explanation of basic concepts and a reference

comment:20 Changed 3 years ago by tscrim

  • Status changed from needs_review to positive_review

Thank you.

comment:21 Changed 3 years ago by vbraun

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