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 )
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
.
Attachments (2)
Change History (21)
comment:1 Changed 6 years ago by
- Dependencies set to #14489, #14746
comment:2 Changed 6 years ago by
- Keywords sd51 added
comment:3 Changed 6 years ago by
- Owner changed from (none) to cremona
comment:4 Changed 6 years ago by
comment:5 Changed 6 years ago by
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
- Cc pbruin added
- Status changed from new to needs_review
comment:7 Changed 6 years ago by
- Cc mkosters akoutsianas added
comment:8 Changed 6 years ago by
- 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
- Description modified (diff)
comment:10 follow-up: ↓ 13 Changed 6 years ago by
- 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
- Description modified (diff)
comment:12 Changed 6 years ago by
- Status changed from needs_review to positive_review
comment:13 in reply to: ↑ 10 Changed 6 years ago by
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: ↓ 15 Changed 6 years ago by
- 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
comment:16 Changed 6 years ago by
- Status changed from needs_work to needs_review
Rebasing done, first patch replaced.
Changed 6 years ago by
comment:17 Changed 6 years ago by
- 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
- Milestone changed from sage-5.12 to sage-5.13
comment:19 Changed 6 years ago by
- Merged in set to sage-5.13.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
I started to work on this at sd51 and hope to finish it soon.