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: sage-8.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:

Status badges

Description (last modified by saraedum)

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 saraedum

Branch: u/saraedum/25607

comment:2 Changed 5 years ago by saraedum

Branch: u/saraedum/25607
Dependencies: #25397

comment:3 Changed 5 years ago by saraedum

Branch: u/saredum/25607

comment:4 Changed 5 years ago by saraedum

Branch: u/saredum/25607u/saraedum/25607
Commit: c1af2bb1ac3c47c3b60c9b40c9d527c4e644d269

New commits:

c1af2bbWIP

comment:5 Changed 5 years ago by git

Commit: c1af2bb1ac3c47c3b60c9b40c9d527c4e644d269e71ad336d242ec20bcdbab23d0ea17435bb78c7e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

5890fddBetter element_with_valuation for function fields
59221baImplement restrictions/element_with_valuation in more places
f87b794Merge remote-tracking branch 'trac/develop' into 25397
076b369Merge remote-tracking branch 'trac/develop' into 25397
3592f74Merge branch '25397' into 25607
e71ad33Shrink coefficients in valuations

comment:6 Changed 5 years ago by saraedum

Description: modified (diff)
Status: newneeds_review
Work issues: is the patchbot happy?

comment:7 Changed 5 years ago by saraedum

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 saraedum

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:9 Changed 5 years ago by swewers

Status: needs_reviewpositive_review

This is great! Thanks.

comment:10 Changed 5 years ago by tscrim

Real name in the reviewers field.

comment:11 Changed 5 years ago by swewers

Reviewers: Stefan Wewers
Status: positive_reviewneeds_work

Soem pyflakes errors.

comment:12 Changed 5 years ago by saraedum

Status: needs_workpositive_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 git

Commit: e71ad336d242ec20bcdbab23d0ea17435bb78c7e8d2324a89be21300206687a265833b5db6e6609f
Status: positive_reviewneeds_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

0650440Merge remote-tracking branch 'trac/develop' into 25607
8d2324aMerge develop and 25607

comment:14 Changed 4 years ago by saraedum

Status: needs_reviewpositive_review

just merged in develop. back to positive review.

comment:15 Changed 4 years ago by vbraun

Branch: u/saraedum/256078d2324a89be21300206687a265833b5db6e6609f
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.