Opened 2 years ago

Closed 19 months ago

#23229 closed enhancement (fixed)

Cache fraction_field() of p-adic rings, deprecate print_mode options

Reported by: saraedum Owned by:
Priority: minor Milestone: sage-8.0
Component: padics Keywords:
Cc: roed Merged in:
Authors: Julian Rüth, David Roe Reviewers: David Roe
Report Upstream: N/A Work issues:
Branch: edf1801 (Commits) Commit: edf180196caab83f360921401e559bd2041698b6
Dependencies: #23510 Stopgaps:

Description (last modified by roed)

Note that creduce calls this a lot since it computes inverse_of_unit in the fraction field.

In a two-step extension, introduced in #23218, this is probably most significant:

sage: w=W.gen()
sage: %timeit u=-w
1000 loops, best of 3: 774 µs per loop # without caching
10000 loops, best of 3: 77.9 µs per loop # with caching

In order to get better speed, this ticket also deprecates the print_mode dictionary that can passed into fraction_field, since this functionality is now available with change.

Attachments (1)

23229_over_23510.diff (3.6 KB) - added by roed 2 years ago.
Diff against #23510 for ease of review

Download all attachments as: .zip

Change History (16)

comment:1 Changed 2 years ago by saraedum

  • Branch set to u/saraedum/cache_fraction_field___of_p_adic_extension_rings

comment:2 Changed 2 years ago by saraedum

  • Commit set to 3b232613682491a3403e3a8f8eeabda8cd0374a5
  • Status changed from new to needs_review

New commits:

3b23261Cache fraction_field()

comment:3 Changed 2 years ago by roed

  • Reviewers set to David Roe
  • Status changed from needs_review to positive_review

Looks good. The doctest failures are due to a missing x-server on Julian's patchbot.

comment:4 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

No, test failures are real...

comment:5 Changed 2 years ago by roed

  • Branch changed from u/saraedum/cache_fraction_field___of_p_adic_extension_rings to u/roed/cache_fraction_field___of_p_adic_extension_rings

comment:6 Changed 2 years ago by git

  • Commit changed from 3b232613682491a3403e3a8f8eeabda8cd0374a5 to 99bdc604d9c200134d8e9014d12b9f82fa635389

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

883e6b5Fix doctest in local_generic_element.pyx
99bdc60Merge branch 't/23510/fixed_mod_frac_field' into t/23229/cache_fraction_field

comment:7 Changed 2 years ago by git

  • Commit changed from 99bdc604d9c200134d8e9014d12b9f82fa635389 to 8224475e0bd5e3cfd30504acf8e02f3ce8409491

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

8224475Add deprecations for integer_ring as well

Changed 2 years ago by roed

Diff against #23510 for ease of review

comment:8 Changed 2 years ago by roed

  • Authors changed from Julian Rüth to Julian Rüth, David Roe
  • Status changed from needs_work to needs_review

I've fixed the doctest and added deprecations for using a print_mode argument for fraction_field. This will speed up the cached_method once we remove it, and the functionality is available through change.

comment:9 Changed 2 years ago by roed

  • Dependencies set to #23510
  • Description modified (diff)
  • Summary changed from Cache fraction_field() of p-adic extension rings to Cache fraction_field() of p-adic rings, deprecate print_mode options

comment:10 Changed 2 years ago by git

  • Commit changed from 8224475e0bd5e3cfd30504acf8e02f3ce8409491 to 2d463cd6574fda86b8cae4e0a7fbe7a9ff6d23e4

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

e51c0f5Merge branch 'u/roed/polynomial_representation_of_a_padic_number' of git://trac.sagemath.org/sage into t/14825/polynomial_representation_of_a_padic_number
b6457b1Moving SEEALSO to the end of the docstring
b81b722Remove use of depraceted list()
04a1579Fix NOTES blocks
dbb869aMerge branch 't/14825/polynomial_representation_of_a_padic_number' into t/23229/cache_frac_field
fee575dMerge branch 't/14825/polynomial_representation_of_a_padic_number' into t/23510/fixed_mod_frac_field
52cbd2aMerge branch 'u/roed/fixed_mod_frac_field' of git://trac.sagemath.org/sage into t/23510/fixed_mod_frac_field
bc59ffaMerge branch 't/14825/polynomial_representation_of_a_padic_number' into t/23510/fixed_mod_frac_field
3292259Merge branch 'u/saraedum/fixed_mod_frac_field' of git://trac.sagemath.org/sage into t/23510/fixed_mod_frac_field
2d463cdMerge branch 'u/saraedum/fixed_mod_frac_field' of git://trac.sagemath.org/sage into t/23229/cache_frac_field

comment:11 Changed 2 years ago by git

  • Commit changed from 2d463cd6574fda86b8cae4e0a7fbe7a9ff6d23e4 to edf180196caab83f360921401e559bd2041698b6

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

6764ea2Merge branch 'develop' into t/14825/polynomial_representation_of_a_padic_number
dad02c7Merge branch 'u/saraedum/fixed_mod_frac_field' of git://trac.sagemath.org/sage into t/23510/fixed_mod_frac_field
189ac2bAdd _test_fraction_field to the coercion tutorial
edf1801Merge branch 't/23510/fixed_mod_frac_field' into t/23229/cache_frac_field

comment:12 Changed 21 months ago by saraedum

Looks good. The patchbots complain about lots of things. If this is just noise from somewhere else you can set this to positive review.

comment:13 Changed 21 months ago by nbruin

Did you check for memory leaks? From pAdicRing(7) is pAdicRing(7) I suspect there is at least some kind of unique representation for padics. This tends to combine very poorly with caching, since it makes unexpected strong global reference chains: Because the ring lives in the (weak value) unique rep. cache, any object it references is strongly globally referenced. If any of those objects now references the ring (for instance by having it in its construction parameters if it's unique rep. itself), you have a globally anchored reference cycle and hence a memory leak.

EDIT: OK, it looks like it's OK, because it seems that the construction parameters of field of fractions or integer ring do not refer to the ring/field, but are both constructed as completions:

sage: R=pAdicRing(7)
sage: R.construction()
(Completion[7], Integer Ring)
sage: R.fraction_field().construction()
(Completion[7], Rational Field)
sage: K=pAdicField(7)
sage: K.construction()
(Completion[7], Rational Field)
sage: K.integer_ring().construction()
(Completion[7], Integer Ring)

It means that "completion" on number fields cannot be caching.

Last edited 20 months ago by nbruin (previous) (diff)

comment:14 Changed 19 months ago by roed

  • Status changed from needs_review to positive_review

I agree that the patchbot failures are just noise, and it sounds like Nils is okay with the caching going on here.

comment:15 Changed 19 months ago by vbraun

  • Branch changed from u/roed/cache_fraction_field___of_p_adic_extension_rings to edf180196caab83f360921401e559bd2041698b6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.