Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hasse-Schmidt derivatives #3912

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

KilianBruns
Copy link

Added

  • Hasse-Schmidt derivatives for polynomials and lists of polynomials

(Implemented at the request of @afkafkafk13)

@afkafkafk13
Copy link
Collaborator

@KilianBruns : Thank you for your contribution.

One hint:
Tests are failing, because the required directory structure in experimental has changed at some point and your code does not respect the new structure yet. 'experimental/HasseSchmidt/HasseSchmidtDerivative.jl' needs to be moved to 'experimental/HasseSchmidt/src/HasseSchmidtDerivative.jl'. Let us see, what the tests say, once the file is in the correct place.

@lgoettgens lgoettgens added the experimental Only changes experimental parts of the code label Jul 4, 2024
Copy link
Collaborator

@afkafkafk13 afkafkafk13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comments mainly as remarks on usability and minor details. The overall idea is fine.

experimental/HasseSchmidt/HasseSchmidtDerivative.jl Outdated Show resolved Hide resolved
experimental/HasseSchmidt/HasseSchmidtDerivative.jl Outdated Show resolved Hide resolved
experimental/HasseSchmidt/HasseSchmidtDerivative.jl Outdated Show resolved Hide resolved
test/HasseSchmidt/HasseSchmidtDerivative.jl Outdated Show resolved Hide resolved
test/HasseSchmidt/HasseSchmidtDerivative.jl Outdated Show resolved Hide resolved
@thofma
Copy link
Collaborator

thofma commented Jul 4, 2024

We usually don't add methods of the form

function hasse_derivatives(v::Vector{<:MPolyRingElem})
  return hasse_derivatives.(v)
end

I would suggest to remove those.

@thofma
Copy link
Collaborator

thofma commented Jul 4, 2024

In all the examples, expressions like

f = R(5*x^2 + 3*y^5);

should be replaced by

f = 5*x^2 + 3*y^5;

@KilianBruns KilianBruns marked this pull request as draft July 7, 2024 18:17
@afkafkafk13
Copy link
Collaborator

Thank you for the changes. They look good.

The test-file still seems to be in the wrong place. The error message reads:
"LoadError: experimental/HasseSchmidt is incomplete: HasseSchmidt/test/ missing or empty. See the documentation at https://docs.oscar-system.org/dev/Experimental/intro/ for details."
Please try to move the test-file and see whether this resolves some of the failing tests.

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.

@afkafkafk13 afkafkafk13 marked this pull request as ready for review September 16, 2024 13:15
@afkafkafk13
Copy link
Collaborator

to be reopened

@afkafkafk13
Copy link
Collaborator

reopen

@afkafkafk13 afkafkafk13 reopened this Sep 16, 2024
Copy link

codecov bot commented Sep 16, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 81.38%. Comparing base (8d957f3) to head (9a4bca6).
Report is 167 commits behind head on master.

Files with missing lines Patch % Lines
experimental/HasseSchmidt/src/HasseSchmidt.jl 75.00% 6 Missing ⚠️
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     
Files with missing lines Coverage Δ
experimental/HasseSchmidt/src/HasseSchmidt.jl 75.00% <75.00%> (ø)

... and 272 files with indirect coverage changes


### Implementation of Hasse-Schmidt derivatives as seen in
###
### Fruehbis-Krueger, Ristau, Schober: 'Embedded desingularization for arithmetic surfaces -- toward a parallel implementation'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice "Hasse-Schmidt derivatives" were either defined or a citation added to a place defining it.

Comment on lines +38 to +40
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])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 $3n+1$ allocations.

return hasse_derivatives(lift(f))
end

# Oscar.MPolyLocRingElem (internal, expert use only)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ################################################
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Stil" ? Or "Still"?

Comment on lines +78 to +80
R, (x, y, z) = polynomial_ring(ZZ, ["x", "y", "z"]);
I = ideal(R, [x^2 - 1]);
RQ, _ = quo(R, I);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for those semicolons here and elsewhere in this test file

Suggested change
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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental Only changes experimental parts of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants