Opened 8 years ago

Closed 6 years ago

#12270 closed enhancement (fixed)

pointless computations when converting number field elements

Reported by: mstreng Owned by: davidloeffler
Priority: major Milestone: sage-5.12
Component: number fields Keywords: sd51
Cc: jdemeyer, daniels, JCooley Merged in: sage-5.12.beta0
Authors: Jenny Cooley Reviewers: David Loeffler
Report Upstream: N/A Work issues:
Branch: u/davidloeffler/ticket/12270 Commit:
Dependencies: Stopgaps:

Description (last modified by mstreng)

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.

Attachments (2)

trac_12270_swap_processes.patch (1.5 KB) - added by JCooley 6 years ago.
swaps the order of the two processes
trac_12270_doc_fix.patch (1011 bytes) - added by JCooley 6 years ago.
fixes an error that came up in testing

Download all attachments as: .zip

Change History (15)

comment:1 follow-up: Changed 8 years ago by jdemeyer

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

comment:2 in reply to: ↑ 1 ; follow-up: Changed 8 years ago by mstreng

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.

comment:3 in reply to: ↑ 2 Changed 8 years ago by jdemeyer

  • Description modified (diff)

Thanks for the clarification. I agree.

comment:4 Changed 8 years ago by mstreng

  • Description modified (diff)

comment:5 Changed 7 years ago by daniels

  • Cc daniels added

comment:6 Changed 6 years ago by yiweishe

  • Cc yiweishe added

comment:7 Changed 6 years ago by JCooley

  • Cc JCooley added; yiweishe removed
  • Keywords sd51 added

Changed 6 years ago by JCooley

swaps the order of the two processes

Changed 6 years ago by JCooley

fixes an error that came up in testing

comment:8 Changed 6 years ago by JCooley

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

comment:9 Changed 6 years ago by JCooley

  • Status changed from new to needs_review

comment:10 Changed 6 years ago by davidloeffler

  • Branch set to u/davidloeffler/ticket/12270

comment:11 Changed 6 years ago by davidloeffler

  • Authors set to Jenny Cooley
  • Reviewers set to David Loeffler
  • Status changed from needs_review to positive_review

Does what it says on the tin. Positive review.

comment:12 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:13 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.12.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.