Opened 6 years ago

Closed 6 years ago

#18740 closed enhancement (fixed)

Reduce overhead for relative number field elements

Reported by: pbruin Owned by:
Priority: minor Milestone: sage-6.9
Component: number fields Keywords: relative number field pari
Cc: Merged in:
Authors: Peter Bruin Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: e59e8ec (Commits, GitHub, GitLab) Commit: e59e8ec7b0ac671167d7df8acd00631cb1fcdc76
Dependencies: #18727, #18739 Stopgaps:

Status badges

Description

We should use the PARI functions wrapped in #18739 to avoid initialisation of PARI rnf structures when only doing simple arithmetic in relative number fields.

Summary of changes:

  • new method NumberFieldElement._pari_polynomial;
  • replace the method _pari_rnfequation of NumberField_relative by _pari_rnfeq, which calls the PARI function nf_rnfeq (which returns slightly more information than rnfequation);
  • replace K.pari_rnf().rnfeltreltoabs(x) by K._pari_rnfeq().eltreltoabs(x) and similar;
  • remove the misleadingly named (and now obsolete) method NumberField_relative._rnfeltreltoabs, which only did something meaningful for elements in the base field;
  • merge NumberField_relative.pari_polynomial into NumberField_generic.pari_polynomial.

Change History (11)

comment:1 Changed 6 years ago by pbruin

  • Branch set to u/pbruin/18740-relative_number_fields
  • Commit set to cddcaafd5aa1d6cdc860a3624ca247283eed985a
  • Status changed from new to needs_review

comment:2 Changed 6 years ago by pbruin

  • Status changed from needs_review to needs_work
  • Work issues set to method names changed in #18739

comment:3 Changed 6 years ago by git

  • Commit changed from cddcaafd5aa1d6cdc860a3624ca247283eed985a to e6dd7a1198fe621fcc3b9c84a20948db2a257307

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

566770aMerge branch 'u/pbruin/18739-pari_rnf_conversion' into 6.8.b5
88fbd3dtrac #18379 doc typo in pari/gen
d27251dtrac #18739 raise into py3 syntax
5373b8eTrac 18739: reviewer comments
08be591Merge branch 'ticket/18739-pari_rnf_conversion' into ticket/18740-relative_number_fields
e6dd7a1Trac 18740: update after renaming of methods in Trac 18739

comment:4 Changed 6 years ago by git

  • Commit changed from e6dd7a1198fe621fcc3b9c84a20948db2a257307 to c661096b45e80a7227eba537ae1f7dc9e3836c1d

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

bf87651Merge branch 'ticket/18739-pari_rnf_conversion' into ticket/18740-relative_number_fields
c661096Trac 18740: reduce overhead for relative number field elements

comment:5 Changed 6 years ago by pbruin

  • Status changed from needs_work to needs_review
  • Work issues method names changed in #18739 deleted

comment:6 follow-up: Changed 6 years ago by vdelecroix

  • Milestone changed from sage-6.8 to sage-6.9
  • Status changed from needs_review to needs_work

Hello Peter,

Instead of

f = pari(self._coefficients()).Polrev()
return f.change_variable_name(name)

you can do

return pari(self._coefficients()).Polrev(name)

that would avoid a copy.

Why did you rename _rnf_equations into _rnfeq. I understand that relative_number_field_equations is a bit long but the Sage standard is more toward no abbreviation at all. Is the name rnfeq standard in PARI/GP?

Vincent

comment:7 in reply to: ↑ 6 ; follow-up: Changed 6 years ago by pbruin

Salut Vincent,

Instead of

f = pari(self._coefficients()).Polrev()
return f.change_variable_name(name)

you can do

return pari(self._coefficients()).Polrev(name)

that would avoid a copy.

OK, I will change this.

Why did you rename _rnf_equations into _rnfeq. I understand that relative_number_field_equations is a bit long but the Sage standard is more toward no abbreviation at all. Is the name rnfeq standard in PARI/GP?

The reason is that _pari_rnfequation() was a wrapper around the PARI/GP function rnfequation(), whereas _pari_rnfeq() wraps the internal PARI library function nf_rnfeq(). It is not just a change of name, but a different method returning similar (but more) output. I think it is best to keep the name of the Sage close to the PARI name. I didn't want to keep a separate method _pari_rnfequation() because it was a private method which isn't used anymore.

comment:8 Changed 6 years ago by git

  • Commit changed from c661096b45e80a7227eba537ae1f7dc9e3836c1d to e59e8ec7b0ac671167d7df8acd00631cb1fcdc76

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

e59e8ecTrac 18740: speed up NumberFieldElement._pari_polynomial()

comment:9 Changed 6 years ago by pbruin

  • Status changed from needs_work to needs_review

comment:10 in reply to: ↑ 7 Changed 6 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to positive_review

Replying to pbruin:

Why did you rename _rnf_equations into _rnfeq. I understand that relative_number_field_equations is a bit long but the Sage standard is more toward no abbreviation at all. Is the name rnfeq standard in PARI/GP?

The reason is that _pari_rnfequation() was a wrapper around the PARI/GP function rnfequation(), whereas _pari_rnfeq() wraps the internal PARI library function nf_rnfeq(). It is not just a change of name, but a different method returning similar (but more) output. I think it is best to keep the name of the Sage close to the PARI name. I didn't want to keep a separate method _pari_rnfequation() because it was a private method which isn't used anymore.

I agree that your way is better.

comment:11 Changed 6 years ago by vbraun

  • Branch changed from u/pbruin/18740-relative_number_fields to e59e8ec7b0ac671167d7df8acd00631cb1fcdc76
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.