Opened 11 years ago

Closed 7 years ago

#10843 closed enhancement (fixed)

Faster listing of number field homsets

Reported by: fwclarke Owned by: davidloeffler
Priority: minor Milestone: sage-6.4
Component: number fields Keywords:
Cc: lftabera, robharron, tfeulner, SimonKing Merged in:
Authors: Francis Clarke Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: e2ee8a3 (Commits, GitHub, GitLab) Commit: e2ee8a3231962e830de11b1fc94598c61fe0c9c1
Dependencies: Stopgaps:

Status badges

Description

The patch speeds up the calculation of homomorphisms between number fields. This is mainly achieved by avoiding unnecessary checking. Doctests are also updated.

Attachments (1)

trac_10843.patch (7.5 KB) - added by fwclarke 9 years ago.
rebased for 5.9

Download all attachments as: .zip

Change History (16)

comment:1 Changed 11 years ago by fwclarke

  • Status changed from new to needs_review

comment:2 Changed 10 years ago by fwclarke

  • Milestone set to sage-5.1

Changed 9 years ago by fwclarke

rebased for 5.9

comment:3 Changed 9 years ago by fwclarke

  • Cc lftabera robharron tfeulner added

The patch has been rebased for Sage-5.9.

comment:4 Changed 9 years ago by SimonKing

  • Cc SimonKing added

comment:5 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:6 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:7 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:8 Changed 8 years ago by vdelecroix

  • Branch set to public/10843
  • Commit set to 51569d96fee90505298606630dc25c87f5d43edf

Hi,

Looks nice.

I uploaded your patch in a git branch and added three commits above:

  • the first one to remove the trailing whitespaces
  • the second one to fits with Sage standard for the docs and make the code more compatible with Python 3
  • the third to have a better caching

Please, tell me what you think.

Vincent


New commits:

20f21a6#10843 faster lising of number field homsets
4040bfdtrac #10843: remove trailing whitespaces
a2abefatrac #10843: documentation, indentation
51569d9trac #10843: improved caching

comment:9 Changed 8 years ago by fwclarke

Thanks for this.  It all seems to be working perfectly.

comment:10 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:11 follow-up: Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Why the separate branches for D absolute/relative? The following works in both cases:

roots = D.polynomial().roots(ring=C, multiplicities=False)

comment:12 Changed 7 years ago by git

  • Commit changed from 51569d96fee90505298606630dc25c87f5d43edf to e2ee8a3231962e830de11b1fc94598c61fe0c9c1

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

946525btrac #10843: remove separate absolute/relative branches
e2ee8a3trac #10843: add check keywords and an associated doctest

comment:13 in reply to: ↑ 11 Changed 7 years ago by fwclarke

  • Status changed from needs_work to needs_review

Replying to jdemeyer:

Why the separate branches for D absolute/relative?

Assuming that you meant C rather than D, I've changed the code as suggested. I don't know why this was done separately. Perhaps the simpler approach didn't work when this ticket began four years ago.

I've made one other change which should have been made before. It solves a problem which arose in http://ask.sagemath.org/question/24173/homomorphisms-for-relative-number-fields/.

comment:14 Changed 7 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:15 Changed 7 years ago by vbraun

  • Branch changed from public/10843 to e2ee8a3231962e830de11b1fc94598c61fe0c9c1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.