Opened 3 years ago
Closed 23 months ago
#23229 closed enhancement (fixed)
Cache fraction_field() of padic rings, deprecate print_mode options
Reported by:  saraedum  Owned by:  

Priority:  minor  Milestone:  sage8.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 )
Note that creduce
calls this a lot since it computes inverse_of_unit
in the fraction field.
In a twostep 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)
Change History (16)
comment:1 Changed 3 years ago by
 Branch set to u/saraedum/cache_fraction_field___of_p_adic_extension_rings
comment:2 Changed 3 years ago by
 Commit set to 3b232613682491a3403e3a8f8eeabda8cd0374a5
 Status changed from new to needs_review
comment:3 Changed 2 years ago by
 Reviewers set to David Roe
 Status changed from needs_review to positive_review
Looks good. The doctest failures are due to a missing xserver on Julian's patchbot.
comment:4 Changed 2 years ago by
 Status changed from positive_review to needs_work
No, test failures are real...
comment:5 Changed 2 years ago by
 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
 Commit changed from 3b232613682491a3403e3a8f8eeabda8cd0374a5 to 99bdc604d9c200134d8e9014d12b9f82fa635389
comment:7 Changed 2 years ago by
 Commit changed from 99bdc604d9c200134d8e9014d12b9f82fa635389 to 8224475e0bd5e3cfd30504acf8e02f3ce8409491
Branch pushed to git repo; I updated commit sha1. New commits:
8224475  Add deprecations for integer_ring as well

comment:8 Changed 2 years ago by
 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
 Dependencies set to #23510
 Description modified (diff)
 Summary changed from Cache fraction_field() of padic extension rings to Cache fraction_field() of padic rings, deprecate print_mode options
comment:10 Changed 2 years ago by
 Commit changed from 8224475e0bd5e3cfd30504acf8e02f3ce8409491 to 2d463cd6574fda86b8cae4e0a7fbe7a9ff6d23e4
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
e51c0f5  Merge branch 'u/roed/polynomial_representation_of_a_padic_number' of git://trac.sagemath.org/sage into t/14825/polynomial_representation_of_a_padic_number

b6457b1  Moving SEEALSO to the end of the docstring

b81b722  Remove use of depraceted list()

04a1579  Fix NOTES blocks

dbb869a  Merge branch 't/14825/polynomial_representation_of_a_padic_number' into t/23229/cache_frac_field

fee575d  Merge branch 't/14825/polynomial_representation_of_a_padic_number' into t/23510/fixed_mod_frac_field

52cbd2a  Merge branch 'u/roed/fixed_mod_frac_field' of git://trac.sagemath.org/sage into t/23510/fixed_mod_frac_field

bc59ffa  Merge branch 't/14825/polynomial_representation_of_a_padic_number' into t/23510/fixed_mod_frac_field

3292259  Merge branch 'u/saraedum/fixed_mod_frac_field' of git://trac.sagemath.org/sage into t/23510/fixed_mod_frac_field

2d463cd  Merge 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
 Commit changed from 2d463cd6574fda86b8cae4e0a7fbe7a9ff6d23e4 to edf180196caab83f360921401e559bd2041698b6
Branch pushed to git repo; I updated commit sha1. New commits:
6764ea2  Merge branch 'develop' into t/14825/polynomial_representation_of_a_padic_number

dad02c7  Merge branch 'u/saraedum/fixed_mod_frac_field' of git://trac.sagemath.org/sage into t/23510/fixed_mod_frac_field

189ac2b  Add _test_fraction_field to the coercion tutorial

edf1801  Merge branch 't/23510/fixed_mod_frac_field' into t/23229/cache_frac_field

comment:12 Changed 2 years ago by
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 2 years ago by
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.
comment:14 Changed 2 years ago by
 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 23 months ago by
 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
New commits:
Cache fraction_field()