Opened 5 years ago
Closed 4 years ago
#25607 closed enhancement (fixed)
Shrink coefficients in Mac Lane algorithm
Reported by:  saraedum  Owned by:  

Priority:  minor  Milestone:  sage8.3 
Component:  padics  Keywords:  
Cc:  swewers  Merged in:  
Authors:  Julian Rüth  Reviewers:  Stefan Wewers 
Report Upstream:  N/A  Work issues:  
Branch:  8d2324a (Commits, GitHub, GitLab)  Commit:  8d2324a89be21300206687a265833b5db6e6609f 
Dependencies:  #25397  Stopgaps: 
Description (last modified by )
As a followup to #25397, let's try to shrink coefficients even further, using the ideas from https://github.com/MCLF/mclf/pull/60/files.
This also speeds up things overall. All the valuation tests took 406s on my machine before and now take 314s.
Change History (15)
comment:1 Changed 5 years ago by
Branch:  → u/saraedum/25607 

comment:2 Changed 5 years ago by
Branch:  u/saraedum/25607 

Dependencies:  → #25397 
comment:3 Changed 5 years ago by
Branch:  → u/saredum/25607 

comment:4 Changed 5 years ago by
Branch:  u/saredum/25607 → u/saraedum/25607 

Commit:  → c1af2bb1ac3c47c3b60c9b40c9d527c4e644d269 
comment:5 Changed 5 years ago by
Commit:  c1af2bb1ac3c47c3b60c9b40c9d527c4e644d269 → e71ad336d242ec20bcdbab23d0ea17435bb78c7e 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
5890fdd  Better element_with_valuation for function fields

59221ba  Implement restrictions/element_with_valuation in more places

f87b794  Merge remotetracking branch 'trac/develop' into 25397

076b369  Merge remotetracking branch 'trac/develop' into 25397

3592f74  Merge branch '25397' into 25607

e71ad33  Shrink coefficients in valuations

comment:6 Changed 5 years ago by
Description:  modified (diff) 

Status:  new → needs_review 
Work issues:  → is the patchbot happy? 
comment:7 Changed 5 years ago by
swewers: If you want to review this (I hope you do ;) This is really a somewhat wild mix of all kinds of ideas and little errors that I found. I can split it up into more isolated bits if you think it's overwhelming like that.
In the end I did not really take any new ideas from your pull request. I think that everything that's in there already existed (just some things did not work/were not called.) If you have any additional ideas in how to improve this, I would be very interested of course :)
comment:8 Changed 5 years ago by
Btw., the doctests in mclf
still works with this. Very few outputs change because the centers of affinoids are now sightly simpler.
Failed example: Y.etale_locus() Expected: Affinoid with 3 components: Elementary affinoid defined by v(x) >= 3/4 Elementary affinoid defined by v(x + 7) >= 5/4 Elementary affinoid defined by v(x + 2) >= 5/4 Got: Affinoid with 3 components: Elementary affinoid defined by v(x) >= 3/4 Elementary affinoid defined by v(x  2) >= 5/4 Elementary affinoid defined by v(x + 2) >= 5/4 <BLANKLINE>
comment:11 Changed 5 years ago by
Reviewers:  → Stefan Wewers 

Status:  positive_review → needs_work 
Soem pyflakes errors.
comment:12 Changed 5 years ago by
Status:  needs_work → positive_review 

Work issues:  is the patchbot happy? 
The pyflakes errors are already fixed in another ticket that touches padic_valuation. Let's ignore them here to avoid merge conflicts.
comment:13 Changed 4 years ago by
Commit:  e71ad336d242ec20bcdbab23d0ea17435bb78c7e → 8d2324a89be21300206687a265833b5db6e6609f 

Status:  positive_review → needs_review 
comment:14 Changed 4 years ago by
Status:  needs_review → positive_review 

just merged in develop. back to positive review.
comment:15 Changed 4 years ago by
Branch:  u/saraedum/25607 → 8d2324a89be21300206687a265833b5db6e6609f 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
WIP