Hasse-Schmidt derivatives #3912
Conversation
…ianBruns/Oscar.jl into kb/hasse_schmidt_derivatives
…ianBruns/Oscar.jl into kb/hasse_schmidt_derivatives
|
@KilianBruns : Thank you for your contribution. One hint: |
|
We usually don't add methods of the form I would suggest to remove those. |
|
In all the examples, expressions like should be replaced by |
|
Thank you for the changes. They look good. The test-file still seems to be in the wrong place. The error message reads: And still one more small request: Please add a small paragraph in the documentation stating briefly what Hasse-Derivatives are and where to find the mathematical background. If you need help with this, let me know by mail and we can look at this together. |
|
to be reopened |
|
reopen |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3912 +/- ##
==========================================
- Coverage 83.99% 81.38% -2.61%
==========================================
Files 591 616 +25
Lines 81358 92691 +11333
==========================================
+ Hits 68333 75439 +7106
- Misses 13025 17252 +4227
|
|
|
||
| ### Implementation of Hasse-Schmidt derivatives as seen in | ||
| ### | ||
| ### Fruehbis-Krueger, Ristau, Schober: 'Embedded desingularization for arithmetic surfaces -- toward a parallel implementation' |
There was a problem hiding this comment.
Perhaps add this to our .bib file so it can be properly referenced in docstrings? (See https://docs.oscar-system.org/stable/DeveloperDocumentation/documentation/#Updating-the-bibliography for hints on how to do that)
| @doc raw""" | ||
| hasse_derivatives(f::MPolyRingElem) | ||
|
|
||
| Return a list of Hasse-Schmidt derivatives of `f`, each with a multiindex `[a_1, ..., a_n]`, where `a_i` describes the number of times `f` was derived w.r.t. the `i`-th variable. |
There was a problem hiding this comment.
It would be nice "Hasse-Schmidt derivatives" were either defined or a citation added to a place defining it.
| Rtemp, _ = polynomial_ring(R, "y" => 1:n, "t" => 1:n) | ||
| # replace f(x_i) -> f(y_i + t_i) | ||
| F = evaluate(f, gens(Rtemp)[1:n] + gens(Rtemp)[n+1:2n]) |
There was a problem hiding this comment.
| Rtemp, _ = polynomial_ring(R, "y" => 1:n, "t" => 1:n) | |
| # replace f(x_i) -> f(y_i + t_i) | |
| F = evaluate(f, gens(Rtemp)[1:n] + gens(Rtemp)[n+1:2n]) | |
| Rtemp, (y, t) = polynomial_ring(R, :y => 1:n, :t => 1:n) | |
| # replace f(x_i) -> f(y_i + t_i) | |
| F = evaluate(f, y + t) |
This avoids allocations.
First generating the polynomial ring is cheaper with symbols instead of strings:
julia> n=2; R=QQ ; @time S,_ = polynomial_ring(R, "y" => 1:n, "t" => 1:n);
0.000070 seconds (85 allocations: 3.562 KiB)
julia> n=2; R=QQ ; @time S,_ = polynomial_ring(R, :y => 1:n, :t => 1:n);
0.000059 seconds (53 allocations: 2.328 KiB)
Secondly, gens(Rtemp) is not free either, it allocates a fresh vector and a fresh bunch of generator polynomials each time. E.g. if the coefficient ring is QQ, then gens(S) above performs
| return hasse_derivatives(lift(f)) | ||
| end | ||
|
|
||
| # Oscar.MPolyLocRingElem (internal, expert use only) |
There was a problem hiding this comment.
Why expert use only? Presumably there is a pitfall, perhaps at least a hint could be added so what that is?
| @@ -0,0 +1,119 @@ | |||
| ###### Stil missing ################################################ | |||
| R, (x, y, z) = polynomial_ring(ZZ, ["x", "y", "z"]); | ||
| I = ideal(R, [x^2 - 1]); | ||
| RQ, _ = quo(R, I); |
There was a problem hiding this comment.
No need for those semicolons here and elsewhere in this test file
| R, (x, y, z) = polynomial_ring(ZZ, ["x", "y", "z"]); | |
| I = ideal(R, [x^2 - 1]); | |
| RQ, _ = quo(R, I); | |
| R, (x, y, z) = polynomial_ring(ZZ, ["x", "y", "z"]) | |
| I = ideal(R, [x^2 - 1]) | |
| RQ, _ = quo(R, I) |
|
What's the status of this PR? It is not marked draft but it also hasn't been updated for 3 months now. |
|
Please mark it as draft and leave it open for the moment (so that he still has easy access to the comments here). I know that @KilianBruns just got back to this. He will open a new PR on this topic within the next week. He is just now moving the stuff to the current status of master and making the requested changes, then he will close this one and open the new one. |
Added
(Implemented at the request of @afkafkafk13)