Opened 6 years ago

Closed 6 years ago

#14963 closed enhancement (fixed)

Add functionality for S-units to UnitGroup

Reported by: cremona Owned by: cremona
Priority: major Milestone: sage-5.13
Component: number fields Keywords: sd51, units, S-units, number fields
Cc: mkosters, akoutsianas Merged in: sage-5.13.beta0
Authors: John Cremona Reviewers: Peter Bruin
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #14489, #14746, #14519 Stopgaps:

Description (last modified by pbruin)

In Sage-5.11, there is a lot of functionality for computing with S-units (see #14746) but no support for this in the UnitGroup class in sage/rings/number_field/unit_group.py. This ticket adds such support, also adding a wrapper for the PARI function bnfissunit.

Apply: trac_14963-Sunits.patch, trac_14963-reviewer.patch

Attachments (2)

trac_14963-Sunits.patch (18.8 KB) - added by cremona 6 years ago.
applies to 5.12.beta1 + #14519 + #14746
trac_14963-reviewer.patch (8.6 KB) - added by pbruin 6 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 6 years ago by cremona

  • Dependencies set to #14489, #14746

comment:2 Changed 6 years ago by cremona

  • Keywords sd51 added

comment:3 Changed 6 years ago by cremona

  • Owner changed from (none) to cremona

comment:4 Changed 6 years ago by cremona

I started to work on this at sd51 and hope to finish it soon.

comment:5 Changed 6 years ago by cremona

It was quite easy to extend the functionality of the UnitGroup? class to include S-units, thanks to the pari function bnfsunit (which was already wrapped) and bnfissunit (which was not). The class constructor requires that S not be a list, but it can be a tuple or something from which an element of the number field can be constructed. Note that the pari function bnfsunit only returns the S-unit generators which are not global units, and the bnfissunit returns the exponents in the order (1) global units of infinite order, (2) torsion, (3) other S-units; so some permutation is required. The new method for number fields is S_unit_group and is simiar to unit_group: it caches S-unit groups using the tuple S as key, with separate caches for proof and non-proof options.

The old number field method S_units is probably now redundant; I added a note there directing users to S_unit_group for great functionality, as we do for units.

comment:6 Changed 6 years ago by cremona

  • Authors set to John Cremona
  • Cc pbruin added
  • Status changed from new to needs_review

comment:7 Changed 6 years ago by cremona

  • Cc mkosters akoutsianas added

comment:8 Changed 6 years ago by pbruin

  • Description modified (diff)
  • Summary changed from Create a new class for S-units to Add functionality for S-units to UnitGroup

update summary and description

comment:9 Changed 6 years ago by pbruin

  • Description modified (diff)

comment:10 follow-up: Changed 6 years ago by pbruin

  • Cc pbruin removed
  • Reviewers set to Peter Bruin

This looks good to me. I like the fact that S-units are now neatly integrated into the UnitGroup class and did not require a separate class.

Did you write this patch on a 32-bit system? I am getting two doctest failures in my 64-bit Sage 5.11.rc0, which are almost certainly due to 32/64-bit differences in PARI (this also occurred in the two dependencies of this ticket).

Apart from that, I can only suggest a few cosmetic changes (mostly trailing whitespace). I will upload a reviewer patch and am planning to give this a positive review once doctesting has finished.

comment:11 Changed 6 years ago by pbruin

  • Description modified (diff)

comment:12 Changed 6 years ago by pbruin

  • Status changed from needs_review to positive_review

comment:13 in reply to: ↑ 10 Changed 6 years ago by cremona

Replying to pbruin:

This looks good to me. I like the fact that S-units are now neatly integrated into the UnitGroup class and did not require a separate class.

Did you write this patch on a 32-bit system? I am getting two doctest failures in my 64-bit Sage 5.11.rc0, which are almost certainly due to 32/64-bit differences in PARI (this also occurred in the two dependencies of this ticket).

Yes, I did this on my laptop at the Leiden Sage Days and that's 32-bit. Sorry for not testing on 64-bit too, and thanks for makinf the necessary fixes. Sorry too for the trailing whitspace in my cut-and-paste doctests, I usually remember to get rid of it.

And thanks for the positive review!

Apart from that, I can only suggest a few cosmetic changes (mostly trailing whitespace). I will upload a reviewer patch and am planning to give this a positive review once doctesting has finished.

comment:14 follow-up: Changed 6 years ago by jdemeyer

  • Dependencies changed from #14489, #14746 to #14489, #14746, #14519
  • Status changed from positive_review to needs_work

This needs to be rebased to #14519.

comment:15 in reply to: ↑ 14 Changed 6 years ago by cremona

Replying to jdemeyer:

This needs to be rebased to #14519.

I will do that, though it is exceedingly annoying to have to do so in order to satisfy some third party whose patch touched hundreds of files and therefore causes many other people such an inconvenience. There should be a fine for that!

Changed 6 years ago by cremona

applies to 5.12.beta1 + #14519 + #14746

comment:16 Changed 6 years ago by cremona

  • Status changed from needs_work to needs_review

Rebasing done, first patch replaced.

Changed 6 years ago by pbruin

comment:17 Changed 6 years ago by pbruin

  • Status changed from needs_review to positive_review

Reviewer patch updated to make the bnfissunit() wrapper use pari_catch_sig_on() (the new name of the PARI version of sig_on(), see #15124).

Tests pass on my 64-bit system, but there might possibly be some doctest failures on 32-bit systems due to different PARI behaviour for 32/64 bits.

comment:18 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.12 to sage-5.13

comment:19 Changed 6 years ago by jdemeyer

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