#29154 closed enhancement (fixed)

Add get_place(degree) to global function fields

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

Status badges

Description (last modified by klee)

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 klee

  • Authors set to Kwankyu Lee
  • Branch set to u/kee/29154

comment:2 Changed 20 months ago by klee

  • Branch changed from u/kee/29154 to u/klee/29154
  • Commit set to f4327839f4bc5e2d44173fa9c90361e3b5d41892

New commits:

f432783Add has_place() to global function fields

comment:3 Changed 20 months ago by klee

  • Status changed from new to needs_review

comment:4 follow-up: Changed 20 months ago by tscrim

  • 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 use-case will always be True as it is a non-empty 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 klee

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 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 use-case will always be True as it is a non-empty tuple).

Good point. Done.

I would also consider renaming this to get_place.

Right. Done.

comment:6 Changed 20 months ago by git

  • Commit changed from f4327839f4bc5e2d44173fa9c90361e3b5d41892 to 99c7b1f9acd5d0ad89e9b518927dba40cfe0614b

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

99c7b1fAdd get_place() to global function fields

comment:7 Changed 20 months ago by klee

  • Status changed from needs_work to needs_review

comment:8 Changed 20 months ago by klee

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

  • Status changed from needs_review to positive_review

Thanks. LGTM.

comment:10 Changed 20 months ago by gh-mwageringel

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

  • Commit changed from 99c7b1f9acd5d0ad89e9b518927dba40cfe0614b to ff1a8378d0b4b7064fdd6e6474e5454bcbd5b4d3

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

ff1a837Fix a typo

comment:12 Changed 20 months ago by klee

  • Status changed from needs_work to positive_review

comment:13 Changed 19 months ago by vbraun

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