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

pointless computations when converting number field elements #12270

Closed
mstreng opened this issue Jan 6, 2012 · 14 comments
Closed

pointless computations when converting number field elements #12270

mstreng opened this issue Jan 6, 2012 · 14 comments

Comments

@mstreng
Copy link
Contributor

mstreng commented Jan 6, 2012

In /sage/rings/number_field/number_field.py (as per #11869), when converting number field elements, the parts

  • List of candidates for K(x)
  • Find a common field F into which KF and LF both embed

are independent. The first can be very slow, while the second is likely to fail. So I propose to swap these two parts.

See #12269 for an example and for indicator code.

CC: @jdemeyer @dansme @sagetrac-JCooley

Component: number fields

Keywords: sd51

Author: Jenny Cooley

Branch: u/davidloeffler/ticket/12270

Reviewer: David Loeffler

Merged: sage-5.12.beta0

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

@jdemeyer
Copy link
Contributor

jdemeyer commented Jan 6, 2012

comment:1

I'm a but confused. Exactly where and when are roots unneccessarily computed?

@mstreng
Copy link
Contributor Author

mstreng commented Jan 6, 2012

comment:2

Replying to @jdemeyer:

I'm a but confused. Exactly where and when are roots unneccessarily computed?

In /sage/rings/number_field/number_field.py (as per #11869) the parts

  • "# List of candidates for K(x)" and
  • "# Find a common field F into which KF and LF both embed."

are independent. The first can be very slow, while the second is likely to fail. So I propose to swap these two parts.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Contributor

jdemeyer commented Jan 6, 2012

comment:3

Thanks for the clarification. I agree.

@mstreng

This comment has been minimized.

@sagetrac-JCooley
Copy link
Mannequin

sagetrac-JCooley mannequin commented Jul 23, 2013

Changed keywords from none to sd51

@sagetrac-JCooley
Copy link
Mannequin

sagetrac-JCooley mannequin commented Jul 24, 2013

swaps the order of the two processes

@sagetrac-JCooley
Copy link
Mannequin

sagetrac-JCooley mannequin commented Jul 24, 2013

Attachment: trac_12270_swap_processes.patch.gz

fixes an error that came up in testing

@sagetrac-JCooley
Copy link
Mannequin

sagetrac-JCooley mannequin commented Jul 24, 2013

comment:8

Attachment: trac_12270_doc_fix.patch.gz

Sorry, newbie error, I did hg_commit() before I had tested the patch, hence there being two patches.

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Jul 24, 2013

Branch: u/davidloeffler/ticket/12270

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Jul 24, 2013

Reviewer: David Loeffler

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Jul 24, 2013

comment:11

Does what it says on the tin. Positive review.

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Jul 24, 2013

Author: Jenny Cooley

@jdemeyer jdemeyer modified the milestones: sage-5.11, sage-5.12 Jul 25, 2013
@jdemeyer
Copy link
Contributor

jdemeyer commented Aug 2, 2013

Merged: sage-5.12.beta0

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

2 participants