Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#14746 closed enhancement (fixed)

Clean up S-class group, S-unit and Selmer group code

Reported by: pbruin Owned by: davidloeffler
Priority: major Milestone: sage-5.12
Component: number fields Keywords: S-class group, S-units, Selmer group, sd51
Cc: ArgaezG, akoutsianas Merged in: sage-5.12.beta2
Authors: Peter Bruin Reviewers: John Cremona, Alejandro Argaez, Angelos Koutsianas
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #14489 Stopgaps:

Description (last modified by pbruin)

The code for S-class groups, S-units and Selmer groups of number fields, and more generally étale algebras, is not entirely satisfactory in the following respects:

  1. The code for computing Selmer groups is somewhat convoluted. Conceptually, the computation of the generators for principal ideals of the form genorder belongs in selmer_group, not _S_class_group_and_units. It would be more correct to return, as S-class group generators, pairs (gen, order) instead of triples (gen, order, pr), and leave the computation of the principal ideal generators to selmer_group.
  1. The docstrings are not very clear. The sentences are very long and contain awkward constructions ("a fractional ideal representative of the S-class group generator whose order (in the S-class group) is order"; "principal generator").
  1. The docstring of NumberField?._S_class_group_and_units suggests that to obtain a principal ideal, genorder can be multiplied by any fractional ideal J whose class is in the subgroup of the class group generated by ideals in S. However, the condition is more strict: J must be in the subgroup of the ideal group generated by ideals in S.

The attached patch does the following things:

  1. Move computation of generators of principal ideals from NumberField?._S_class_group_and_units to NumberField?.selmer_group.
  1. Add a method _S_decomposition to PolynomialQuotientRing_generic, which computes the decomposition of an étale algebra as a product of number fields. Use this function in S_class_group, S_units and selmer_group.
  1. Delete PolynomialQuotientRing_generic._S_class_group_and_units, and move its code and doctests to S_class_group, class_group, S_units and units.
  1. Reimplement PolynomialQuotientRing_generic.selmer_group to compute the Selmer group as the product of the Selmer groups of the distinct components, instead of imitating the algorithm of NumberField?.selmer_group.
  1. Make the documentation more precise.

Apply: trac_14746_doctests_32_64_bit_undo.patch, trac_14746_selmer_group_cleanup.patch, trac_14746_docstring_fixes.patch

Attachments (3)

trac_14746_selmer_group_cleanup.patch (33.6 KB) - added by pbruin 6 years ago.
based on 5.10.rc1 + #14489
trac_14746_doctests_32_64_bit_undo.patch (3.4 KB) - added by pbruin 6 years ago.
undo the effect of trac_14489_doctests_32_64_bit.patch
trac_14746_docstring_fixes.patch (3.5 KB) - added by pbruin 6 years ago.
add doctest, utf-8 declaration, r""" for backslashes, trivial edit in code

Download all attachments as: .zip

Change History (25)

Changed 6 years ago by pbruin

based on 5.10.rc1 + #14489

comment:1 Changed 6 years ago by pbruin

  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 6 years ago by pbruin

  • Description modified (diff)

Changed 6 years ago by pbruin

undo the effect of trac_14489_doctests_32_64_bit.patch

comment:3 Changed 6 years ago by pbruin

  • Description modified (diff)

comment:4 Changed 6 years ago by pbruin

For patchbot:

Apply trac_14746_doctests_32_64_bit_undo.patch, trac_14746_selmer_group_cleanup.patch

Last edited 6 years ago by pbruin (previous) (diff)

comment:5 Changed 6 years ago by pbruin

  • Description modified (diff)

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

The source file needs to specify UTF-8 encoding if you want to use non-ascii characters, but rings/polynomial/polynomial_quotient_ring.py does not. See http://www.python.org/dev/peps/pep-0263/

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

Replying to vbraun:

The source file needs to specify UTF-8 encoding if you want to use non-ascii characters, but rings/polynomial/polynomial_quotient_ring.py does not. See http://www.python.org/dev/peps/pep-0263/

My patch fixes that as well, by adding a line # -*- coding: utf-8 -*- at the beginning (I forgot to mention this in the patch description). Does this conform to the expected syntax?

comment:8 Changed 6 years ago by pbruin

After checking the documentation, I doubt that the 'raw' declaration (changing """ to r""") is the right fix. Should u""" be used instead, or is """ OK as long as the file is declared to be UTF-8?

comment:9 Changed 6 years ago by vbraun

OK, I didn't see the last patch in context. The raw only concerns backlashes but handy to not have to escape \TeX commands. No need for unicode strings.

comment:10 Changed 6 years ago by pbruin

I removed the second part of the patch, which changed """ to r""".

comment:11 follow-up: Changed 6 years ago by cremona

I hope I can look at this and get it finished next week. Does it provide a class for S-units like the class for units?

comment:12 follow-up: Changed 6 years ago by vbraun

Test failure in my 32-bit VM:

File "sage/rings/polynomial/polynomial_quotient_ring.py", line 926, in sage.rings.polynomial.polynomial_quotient_ring.PolynomialQuotientRing_generic._S_decomposition
Failed example:
    S._S_decomposition(tuple(K.primes_above(3)))
Expected:
    ([Number Field in x0 with defining polynomial x^2 + 23 over its base field,
      Number Field in x1 with defining polynomial x^2 + 31 over its base field],
     [(Ring endomorphism of Number Field in y0 with defining polynomial x^4 + 56*x^2 + 324
      Defn: y0 |--> y0,
       0),
      (Ring endomorphism of Number Field in y1 with defining polynomial x^4 + 72*x^2 + 676
      Defn: y1 |--> y1,
       1)],
     [(Number Field in y0 with defining polynomial x^4 + 56*x^2 + 324,
       [Fractional ideal (3, 1/4*y0^2 + 1/2*y0 + 11/2),
        Fractional ideal (3, 1/72*y0^3 + 1/4*y0^2 + 19/36*y0 + 8),
        Fractional ideal (3, 1/36*y0^3 + 1/4*y0^2 + 5/9*y0 + 13/2),
        Fractional ideal (3, 1/72*y0^3 + 1/4*y0^2 + 19/36*y0 + 6)]),
      (Number Field in y1 with defining polynomial x^4 + 72*x^2 + 676,
       [Fractional ideal (3, -1/52*y1^3 - 23/26*y1 - 1),
        Fractional ideal (3, -1/52*y1^3 - 23/26*y1 + 1)])])
Got:
    ([Number Field in x0 with defining polynomial x^2 + 23 over its base field, Number Field in x1 with defining polynomial x^2 + 31 over its base field], [(Ring endomorphism of Number Field in y0 with defining polynomial x^4 + 56*x^2 + 324
      Defn: y0 |--> y0, 0), (Ring endomorphism of Number Field in y1 with defining polynomial x^4 + 72*x^2 + 676
      Defn: y1 |--> y1, 1)], [(Number Field in y0 with defining polynomial x^4 + 56*x^2 + 324, [Fractional ideal (3, -1/4*y0^2 - 1/2*y0 - 11/2), Fractional ideal (3, 1/72*y0^3 + 1/4*y0^2 + 19/36*y0 + 8), Fractional ideal (3, 1/36*y0^3 - 1/4*y0^2 + 14/9*y0 - 17/2), Fractional ideal (3, 1/72*y0^3 + 1/4*y0^2 + 19/36*y0 + 6)]), (Number Field in y1 with defining polynomial x^4 + 72*x^2 + 676, [Fractional ideal (3, -1/52*y1^3 - 23/26*y1 - 1), Fractional ideal (3, -1/52*y1^3 - 23/26*y1 + 1)])])

Edit: sorry had the wrong ticket number, this is the failure I'm getting.

Last edited 6 years ago by vbraun (previous) (diff)

comment:13 in reply to: ↑ 11 Changed 6 years ago by pbruin

Replying to cremona:

I hope I can look at this and get it finished next week. Does it provide a class for S-units like the class for units?

It doesn't add new (user-level) functionality, but the clean-up might make it easier to add this in the future.

comment:14 in reply to: ↑ 12 Changed 6 years ago by pbruin

Replying to vbraun:

Test failure in my 32-bit VM:

With hindsight I could have guessed that the new doctest would create another 32/64-bit difference... I am preparing a patch that

  • adds a bit more explanation to the docstring;
  • instead of checking the generators of the primes lying above 3, just checks whether the number of such primes is correct;
  • makes a trivial simplification in the code;
  • changes back """ to r""" in two docstrings, since they contain backslashes.
Last edited 6 years ago by pbruin (previous) (diff)

Changed 6 years ago by pbruin

add doctest, utf-8 declaration, r""" for backslashes, trivial edit in code

comment:15 Changed 6 years ago by davidloeffler

  • Keywords sd51 added

comment:16 Changed 6 years ago by cremona

All three patches apply fine to (5.11.beta3 + the two patches from #14489). Most of the changes here are cosmetic, moving code around and improving the documentation, and they look fine to me. Testing now...

comment:17 Changed 6 years ago by cremona

  • Cc ArgaezG akoutsianas added
  • Reviewers set to John Cremona, Alejandro Argaez, Angelos Koutsianas

comment:18 Changed 6 years ago by cremona

All tests (including long) pass in sage/rings, so I'll give this a positive review.

comment:19 Changed 6 years ago by cremona

  • Status changed from needs_review to positive_review

comment:20 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:21 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.12.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:22 Changed 5 years ago by cremona

See #16496.

Note: See TracTickets for help on using tickets.