Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#27949 closed enhancement (fixed)

Pull out subfield() method from subfields() method.

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

Status badges

Description

It is currently clumsy to create a subfield of a finite field using subfields() method. 

sage: k = GF(2^21)
sage: k.subfields(3)
[(Finite Field in z3 of size 2^3, Ring morphism:
    From: Finite Field in z3 of size 2^3
    To:   Finite Field in z21 of size 2^21
    Defn: z3 |--> z21^20 + z21^19 + z21^17 + z21^15 + z21^11 + z21^9 + z21^8 + z21^6 + z21^2)]
sage: K, _ = k.subfields(3)[0]
sage: K
Finite Field in z3 of size 2^3

With the patch of this ticket, we have subfield() method such that 

sage: k = GF(2^21)
sage: k.subfield(3)
Finite Field in z3 of size 2^3
sage: K = _
sage: k.coerce_map_from(K)
Ring morphism:
  From: Finite Field in z3 of size 2^3
  To:   Finite Field in z21 of size 2^21
  Defn: z3 |--> z21^20 + z21^19 + z21^17 + z21^15 + z21^11 + z21^9 + z21^8 + z21^6 + z21^2

Change History (17)

comment:1 Changed 3 years ago by klee

  • Branch set to u/klee/27949

comment:2 Changed 3 years ago by git

  • Commit set to 87179a794b70ed68b22d5ad79d1b710948544701

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

​87179a7Pull out subfield() from subfields()

comment:3 Changed 3 years ago by klee

  • Status changed from new to needs_review

comment:4 Changed 3 years ago by embray

  • Milestone changed from sage-8.8 to sage-8.9

Moving tickets from the Sage 8.8 milestone that have been actively worked on in the last six months to the next release milestone (optimistically).

comment:5 Changed 3 years ago by git

  • Commit changed from 87179a794b70ed68b22d5ad79d1b710948544701 to 30e85b3270327de07eab28d75780ba110c5d8a0e

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

​30e85b3Merge branch 'develop'

comment:6 Changed 3 years ago by git

  • Commit changed from 30e85b3270327de07eab28d75780ba110c5d8a0e to 147a412b31a73022220b267e0cc3a7e85ff1baf0

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

​147a412Pull out subfield() from subfields()

comment:7 Changed 3 years ago by tscrim

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

LGTM.

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

  • Status changed from positive_review to needs_info

Sorry, I just noticed something: 

return [self.subfields(m, name=name[m])[0] for m in divisors]

Couldn't this use your new subfield() method?

comment:9 in reply to: ↑ 8 ; follow-up: Changed 3 years ago by klee

Replying to tscrim:

Sorry, I just noticed something: 

return [self.subfields(m, name=name[m])[0] for m in divisors]

Couldn't this use your new subfield() method?

Then it would look like 

-        return [self.subfields(m, name=name[m])[0] for m in divisors]
+
+        pairs = []
+        for m in divisors:
+            K =  self.subfield(m, name=name[m])
+            pairs.append( (K, self.coerce_map_from(K)) )
+        return pairs

It is somewhat duplicating the code above. Is this better?

comment:10 in reply to: ↑ 9 Changed 3 years ago by tscrim

Replying to klee:

Replying to tscrim:

Sorry, I just noticed something: 

return [self.subfields(m, name=name[m])[0] for m in divisors]

Couldn't this use your new subfield() method?

Then it would look like 

-        return [self.subfields(m, name=name[m])[0] for m in divisors]
+
+        pairs = []
+        for m in divisors:
+            K =  self.subfield(m, name=name[m])
+            pairs.append( (K, self.coerce_map_from(K)) )
+        return pairs

It is somewhat duplicating the code above. Is this better?

Maybe. It seems less wasteful in terms of object creation and it is not too much code duplication. Plus, the current version almost seems to to go against the new method; although I guess not exactly from what you're saying.

comment:11 Changed 3 years ago by git

  • Commit changed from 147a412b31a73022220b267e0cc3a7e85ff1baf0 to bd8ba100ef581c7a71a93a8e986db42721a3fb43

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

​bd8ba10Use subfield() in subfields() method

comment:12 Changed 3 years ago by klee

  • Status changed from needs_info to needs_review

comment:13 Changed 3 years ago by tscrim

  • Status changed from needs_review to positive_review

Thank you.

comment:14 Changed 3 years ago by klee

My pleasure. Thank you!

comment:15 Changed 3 years ago by chapoton

  • Milestone changed from sage-8.9 to sage-9.0

moving milestone to 9.0 (after release of 8.9)

comment:16 Changed 3 years ago by vbraun

  • Branch changed from u/klee/27949 to bd8ba100ef581c7a71a93a8e986db42721a3fb43
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:17 Changed 2 years ago by mkoeppe

  • Commit bd8ba100ef581c7a71a93a8e986db42721a3fb43 deleted

Follow-up: #30171

Note: See TracTickets for help on using tickets.