Add functionality for S-units to UnitGroup

[JohnCremona]

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: attachment: trac_14963-Sunits.patch, attachment: trac_14963-reviewer.patch

Depends on #14489
Depends on #14746
Depends on #14519

CC: @sagetrac-mkosters @sagetrac-akoutsianas

Component: number fields

Keywords: sd51, units, S-units

Author: John Cremona

Reviewer: Peter Bruin

Merged: sage-5.13.beta0

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

[JohnCremona]

Dependencies: #14489, #14746

[JohnCremona]

Changed keywords from units, S-units to sd51, units, S-units

[JohnCremona]

comment:4

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

[JohnCremona]

comment:5

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.

[JohnCremona]

Author: John Cremona

[pjbruin]

comment:8

update summary and description

[pjbruin]

comment:10

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.

[pjbruin]

Reviewer: Peter Bruin

[JohnCremona]

comment:13

Replying to @pjbruin:

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.

[jdemeyer]

comment:14

This needs to be rebased to #14519.

[jdemeyer]

Changed dependencies from #14489, #14746 to #14489, #14746, #14519

[JohnCremona]

comment:15

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!

[JohnCremona]

Attachment: trac_14963-Sunits.patch.gz

applies to 5.12.beta1 + #14519 + #14746

[JohnCremona]

comment:16

Rebasing done, first patch replaced.

[pjbruin]

Attachment: trac_14963-reviewer.patch.gz

[pjbruin]

comment:17

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.

[jdemeyer]

Merged: sage-5.13.beta0