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

Further improvements to splitting_field() #15626

Closed
jdemeyer opened this issue Jan 3, 2014 · 17 comments
Closed

Further improvements to splitting_field() #15626

jdemeyer opened this issue Jan 3, 2014 · 17 comments

Comments

@jdemeyer
Copy link

jdemeyer commented Jan 3, 2014

  1. Implement an "early abort" option to stop if it's clear that the degree will be huge.
  2. Add some more examples (more indirect examples will be added by Implement division_field() for elliptic curves #11905)

Depends on #2217

CC: @JohnCremona @fchapoton

Component: number fields

Author: Jeroen Demeyer

Branch/Commit: u/jdemeyer/ticket/15626 @ 776795d

Reviewer: John Cremona

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

@jdemeyer jdemeyer added this to the sage-6.1 milestone Jan 3, 2014
@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

jdemeyer commented Jan 3, 2014

Branch: u/jdemeyer/ticket/15626

@jdemeyer
Copy link
Author

jdemeyer commented Jan 3, 2014

New commits:

8f0baaeFurther improvements to splitting_field()
c50eb3eNo need to specify caller_name in verbose()
6df69b6Add comments, small cosmetic changes
9e2dd44Make q monic before computing cubic resolvent
9b5558eImplement splitting fields for number fields

@jdemeyer
Copy link
Author

jdemeyer commented Jan 3, 2014

Commit: 8f0baae

@jdemeyer
Copy link
Author

jdemeyer commented Jan 3, 2014

Dependencies: #2217

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 3, 2014

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

dd4fe5f Further improvements to splitting_field()
c50eb3e No need to specify caller_name in verbose()
6df69b6 Add comments, small cosmetic changes
9e2dd44 Make q monic before computing cubic resolvent
9b5558e Implement splitting fields for number fields

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 3, 2014

Changed commit from 8f0baae to dd4fe5f

@jdemeyer
Copy link
Author

jdemeyer commented Jan 3, 2014

Changed dependencies from #2217 to none

@jdemeyer
Copy link
Author

jdemeyer commented Jan 3, 2014

Dependencies: #2217

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 3, 2014

Changed commit from dd4fe5f to df52508

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 3, 2014

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

df52508 Further improvements to splitting_field()
c50eb3e No need to specify caller_name in verbose()
6df69b6 Add comments, small cosmetic changes
9e2dd44 Make q monic before computing cubic resolvent
9b5558e Implement splitting fields for number fields

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 3, 2014

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

776795dDo polynomial consistency check only for minimal dm

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 3, 2014

Changed commit from df52508 to 776795d

@jdemeyer

This comment has been minimized.

@JohnCremona
Copy link
Member

comment:10

Note to self as reviewer: commits up to c50eb3e have been merged in #2217, so the ones to review here are: df52508, 776795d.

@JohnCremona
Copy link
Member

Reviewer: John Cremona

@JohnCremona
Copy link
Member

comment:11

The Abort class is a great idea and very well implemented. It would never have occurred to me to implement the early abort this way, and it has led me to think about other situations where a similar strategy might be useful! The examples are good.

All tests in sage/rings pass. For some reason when I tested the whole of Sage built with this branch I got various doctest errors in sage/crypto but I cannot believe that it has anything to do with these changes, so they are not stopping me giving it a positive review. Now I am looking forward to looking at the elliptic curve division field code!

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

3 participants