Replace some setprecision!(::ParentObject)
#1522
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1522 +/- ##
==========================================
- Coverage 75.37% 75.28% -0.10%
==========================================
Files 354 354
Lines 112815 112827 +12
==========================================
- Hits 85032 84938 -94
- Misses 27783 27889 +106
|
There was a problem hiding this comment.
This PR is big and has been sitting here since May. I tried to give it a review, but the precision changes seem rather delicate to me, and there are so many changes in here, most unrelated, that I gave up halfway through :-(. Sorry about that (I am not the most qualified person for this anyway, so perhaps not a big loss).
But I noticed that the "change constructors to snake case" part is huge, yet mostly trivial. Perhaps that could be split into a separate PR (and perhaps some of the other precision related changes into yet another) which then might make each individual PR easier and quicker to review?
| Zp = maximal_order(Qp) | ||
| Qq, gQq = QadicField(minimum(P), f, prec_padics, cached = false) | ||
| Qq, gQq = unramified_extension(Qp, f, precision = prec_padics, cached = false) |
There was a problem hiding this comment.
I have no detail about the maths in here, so I just trust this change (which unlike the others seems not completely "obvious") is correct
| r = with_precision(Q, p) do | ||
| return r - qf(r)*o | ||
| end | ||
| if precision(r) >= n |
There was a problem hiding this comment.
It seems to me that before, the precision of r resp. Q was always restricted to n if precision(r) >= n, while now it might stay at a larger value. Presumably this is fine, though, or even an improvement?
Unfortunately this particular case does not seem to be covered by CI.
| for x = lf | ||
| if is_splitting(C) || degree(x[1]) == degree(Q) | ||
| append!(rt, roots(Q, x[1], max_roots = 1)) | ||
| with_precision(Q, n) do |
There was a problem hiding this comment.
would it be better to move the with_precision outside the for x = lf loop?
Oh, sorry, I thought this was marked as draft. I didn't really proceed with this: there is a lot of code that apparently assumes that the precision of some field is set correctly at some point before. Since I don't know half of the involved theory, most of this was educated guessing and trial and error with the CI tests. Splitting off the trivial renamings and such sounds like a good idea. I'll look into it. |
joschmitt
force-pushed
the
js/padicsagain
branch
from
f2ab7d9
to
ac03988
Compare
|
I rebased this now on top of #1605. There seems to be a new test failure (I have the vague feeling that I once debugged this one already, apparently it is back). And it still breaks OSCAR of course. If people are interested in this and there is nothing better to do for me, I can try to get this and a corresponding OSCAR pull request done during the coding sprint next week. Possibly with the help of some of people who know a bit more about |
This replaces many
setprecision!(::ParentObject)calls bywith_precisionblocks.Also removesprime_fieldin the context of local fields.