Opened 20 months ago
Closed 19 months ago
#29154 closed enhancement (fixed)
Add get_place(degree) to global function fields
Reported by:  klee  Owned by:  

Priority:  minor  Milestone:  sage9.1 
Component:  algebra  Keywords:  
Cc:  Merged in:  
Authors:  Kwankyu Lee  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  ff1a837 (Commits, GitHub, GitLab)  Commit:  ff1a8378d0b4b7064fdd6e6474e5454bcbd5b4d3 
Dependencies:  Stopgaps: 
Description (last modified by )
Implemented a new method get_place(degree)
to quickly get an arbitrary place of given degree
Change History (13)
comment:1 Changed 20 months ago by
 Branch set to u/kee/29154
comment:2 Changed 20 months ago by
 Branch changed from u/kee/29154 to u/klee/29154
 Commit set to f4327839f4bc5e2d44173fa9c90361e3b5d41892
comment:3 Changed 20 months ago by
 Status changed from new to needs_review
comment:4 followup: ↓ 5 Changed 20 months ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to needs_work
I think it would be better to just return the place if it exists and None
otherwise. This makes testing much easier because you can do things like
if L.has_place(1): print("success")
and
p = L.has_place(1) if p: return p
without having to extract the information (note that the first usecase will always be True
as it is a nonempty tuple). I would also consider renaming this to get_place
and if you want a simple binary test function:
def has_place(self, degree): return self.get_place(degree) is not None
comment:5 in reply to: ↑ 4 Changed 20 months ago by
Replying to tscrim:
I think it would be better to just return the place if it exists and
None
otherwise. This makes testing much easier because you can do things likeif L.has_place(1): print("success")and
p = L.has_place(1) if p: return pwithout having to extract the information (note that the first usecase will always be
True
as it is a nonempty tuple).
Good point. Done.
I would also consider renaming this to
get_place
.
Right. Done.
comment:6 Changed 20 months ago by
 Commit changed from f4327839f4bc5e2d44173fa9c90361e3b5d41892 to 99c7b1f9acd5d0ad89e9b518927dba40cfe0614b
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
99c7b1f  Add get_place() to global function fields

comment:7 Changed 20 months ago by
 Status changed from needs_work to needs_review
comment:8 Changed 20 months ago by
 Description modified (diff)
 Summary changed from Add has_place(degree) to global function fields to Add get_place(degree) to global function fields
comment:9 Changed 20 months ago by
 Status changed from needs_review to positive_review
Thanks. LGTM.
comment:10 Changed 20 months ago by
 Status changed from positive_review to needs_work
The documentation does not build due to this line:
+ Return a place of ``degree`.
comment:11 Changed 20 months ago by
 Commit changed from 99c7b1f9acd5d0ad89e9b518927dba40cfe0614b to ff1a8378d0b4b7064fdd6e6474e5454bcbd5b4d3
Branch pushed to git repo; I updated commit sha1. New commits:
ff1a837  Fix a typo

comment:12 Changed 20 months ago by
 Status changed from needs_work to positive_review
comment:13 Changed 19 months ago by
 Branch changed from u/klee/29154 to ff1a8378d0b4b7064fdd6e6474e5454bcbd5b4d3
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Add has_place() to global function fields