Opened 3 years ago
Closed 3 years ago
#26616 closed enhancement (fixed)
Global function fields: places
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:  8049919 (Commits, GitHub, GitLab)  Commit:  80499197cf1facfc7757c824b2fd9055d10a2a49 
Dependencies:  Stopgaps: 
Description
This is part of the metaticket #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
 Branch set to u/klee/26616
comment:2 Changed 3 years ago by
 Commit set to 2531d4aa50da176f4ac33025151f8fc5168a0a2c
comment:3 Changed 3 years ago by
 Commit changed from 2531d4aa50da176f4ac33025151f8fc5168a0a2c to 2e6ebee5d9fa86c6b71f117f228c0388f277dce4
comment:4 Changed 3 years ago by
 Status changed from new to needs_review
comment:5 Changed 3 years ago by
 Commit changed from 2e6ebee5d9fa86c6b71f117f228c0388f277dce4 to b0773fbe31efda1bbc93c35830ffce3e32279bf5
Branch pushed to git repo; I updated commit sha1. New commits:
b0773fb  Remove unused _cache

comment:6 Changed 3 years ago by
 Commit changed from b0773fbe31efda1bbc93c35830ffce3e32279bf5 to 1d8d0a6d45ddc5f6b8c93b52d5ddb5b3d82fe998
Branch pushed to git repo; I updated commit sha1. New commits:
1d8d0a6  Remove unnecessry @cahced_method

comment:7 Changed 3 years ago by
comment:8 Changed 3 years ago by
 Commit changed from 1d8d0a6d45ddc5f6b8c93b52d5ddb5b3d82fe998 to ecc74051b0eb544787be617ffb26e2874d789122
Branch pushed to git repo; I updated commit sha1. New commits:
ecc7405  Remove unused import

comment:9 Changed 3 years ago by
 Commit changed from ecc74051b0eb544787be617ffb26e2874d789122 to b117c23c54b219af0309b1f12f913e9710f0ae8f
Branch pushed to git repo; I updated commit sha1. New commits:
b117c23  Merge branch 'develop'

comment:10 Changed 3 years ago by
 Commit changed from b117c23c54b219af0309b1f12f913e9710f0ae8f to b23cc8e796d75b8c00e497b92fb7214093f5b0a7
Branch pushed to git repo; I updated commit sha1. New commits:
b23cc8e  Fix doctests that randomly fail

comment:11 Changed 3 years ago by
 Commit changed from b23cc8e796d75b8c00e497b92fb7214093f5b0a7 to d83ddd09106ddc38c242a92c6eb5436c4b09f352
Branch pushed to git repo; I updated commit sha1. New commits:
d83ddd0  Merge branch 'develop'

comment:12 Changed 3 years ago by
 Commit changed from d83ddd09106ddc38c242a92c6eb5436c4b09f352 to 69f866f20f0f4a444a1a1e6f91d7425cfbd06e86
Branch pushed to git repo; I updated commit sha1. New commits:
69f866f  Merge branch 'develop'

comment:13 followup: ↓ 14 Changed 3 years ago by
Some comments:
 Perhaps it is my lack of understanding of the field, but is
place
common terminology forvaluation()
? 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)]
withlist(self._places_finite(degree))
. Similarly for theinfinite
.  I think there is a grammar issue here:
Return the unique place at infinite.
Should it beReturn 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 asself._gens_over_base
instead ofself._gens_over_base()
.)  I am not sure about this change:
as it might be slower. However, this is a minor thing (the new code is definitely more readable).
 norm = reduce(operator.mul, hnf.diagonal()) + norm = 1 + for e in hnf.diagonal(): + norm *= e
 I do not think you need the
lazy_import('sage.matrix.constructor', 'matrix')
insage/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
Replying to tscrim:
Some comments:
 Perhaps it is my lack of understanding of the field, but is
place
common terminology forvaluation()
? 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, onetoone, 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 onetoone 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)]
withlist(self._places_finite(degree))
. Similarly for theinfinite
.
Done. Thanks.
 I think there is a grammar issue here:
Return the unique place at infinite.
Should it beReturn 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 asself._gens_over_base
instead ofself._gens_over_base()
.)
Ok. Done. Thanks for a good tip.
 I am not sure about this change:
as it might be slower. However, this is a minor thing (the new code is definitely more readable). norm = reduce(operator.mul, hnf.diagonal()) + norm = 1 + for e in hnf.diagonal(): + norm *= e
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')
insage/rings/function_field/valuation_ring.py
.
Removed.
Thank you for giving attention!!
comment:15 Changed 3 years ago by
 Commit changed from 69f866f20f0f4a444a1a1e6f91d7425cfbd06e86 to 8e86177f0b3187c3d0643e9f6565d3110d99be02
comment:16 Changed 3 years ago by
 Commit changed from 8e86177f0b3187c3d0643e9f6565d3110d99be02 to 97d3bc384f9ff05e4a2cd881530998d897da291b
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
97d3bc3  Add places to global function fields

comment:17 Changed 3 years ago by
Squashed and rebased to sage 8.6.
comment:18 Changed 3 years ago by
 Milestone changed from sage8.5 to sage8.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 modulelevel or in some key classlevel 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
 Commit changed from 97d3bc384f9ff05e4a2cd881530998d897da291b to 80499197cf1facfc7757c824b2fd9055d10a2a49
Branch pushed to git repo; I updated commit sha1. New commits:
8049919  Add some explanation of basic concepts and a reference

comment:21 Changed 3 years ago by
 Branch changed from u/klee/26616 to 80499197cf1facfc7757c824b2fd9055d10a2a49
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Add place