Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
kwankyu opened this issue Jun 8, 2019 · 21 comments
Closed

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

kwankyu opened this issue Jun 8, 2019 · 21 comments

Comments

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 8, 2019

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

Component: finite rings

Author: Kwankyu Lee

Branch: bd8ba10

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/27949

@kwankyu kwankyu added this to the sage-8.8 milestone Jun 8, 2019
@kwankyu
Copy link
Collaborator Author

kwankyu commented Jun 8, 2019

Branch: u/klee/27949

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 8, 2019

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

87179a7Pull out subfield() from subfields()

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 8, 2019

Commit: 87179a7

@embray
Copy link
Contributor

embray commented Jul 3, 2019

comment:4

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).

@embray embray modified the milestones: sage-8.8, sage-8.9 Jul 3, 2019
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 4, 2019

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

30e85b3Merge branch 'develop'

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 4, 2019

Changed commit from 87179a7 to 30e85b3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 16, 2019

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

147a412Pull out subfield() from subfields()

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 16, 2019

Changed commit from 30e85b3 to 147a412

@tscrim
Copy link
Collaborator

tscrim commented Sep 18, 2019

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Sep 18, 2019

comment:7

LGTM.

@tscrim
Copy link
Collaborator

tscrim commented Sep 18, 2019

comment:8

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?

@kwankyu
Copy link
Collaborator Author

kwankyu commented Sep 18, 2019

comment:9

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?

@tscrim
Copy link
Collaborator

tscrim commented Sep 18, 2019

comment:10

Replying to @kwankyu:

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2019

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

bd8ba10Use subfield() in subfields() method

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2019

Changed commit from 147a412 to bd8ba10

@tscrim
Copy link
Collaborator

tscrim commented Sep 18, 2019

comment:13

Thank you.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Sep 18, 2019

comment:14

My pleasure. Thank you!

@fchapoton
Copy link
Contributor

comment:15

moving milestone to 9.0 (after release of 8.9)

@fchapoton fchapoton modified the milestones: sage-8.9, sage-9.0 Sep 30, 2019
@vbraun
Copy link
Member

vbraun commented Oct 6, 2019

Changed branch from u/klee/27949 to bd8ba10

@mkoeppe
Copy link
Member

mkoeppe commented Jul 18, 2020

comment:17

Follow-up: #30171

@mkoeppe
Copy link
Member

mkoeppe commented Jul 18, 2020

Changed commit from bd8ba10 to none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants