Skip to content

feat: Groebner Walk #3131

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

Closed
wants to merge 2 commits into from
Closed

feat: Groebner Walk #3131

wants to merge 2 commits into from

Conversation

welpj
Copy link

@welpj welpj commented Dec 21, 2023

Hello @ederc ,
I finished the first step of the migration of the Groebner Walk to the Oscar.jl library and already talked to @afkafkafk13 about it.

This PR includes:

  • Gröbner Walk
  • Perturbed Gröbner Walk
  • Fractal Walk
  • Generic Walk
  • Tran´s version of the Groebner Walk

There probably are several improvements necessary, for example in the use of IdealGens: I don't know the handling of IdealGens in detail, @jankoboehm do you have any hints for improvements?

At the moment the tests take less than two minutes, but I was thinking of adding another bigger Ideal for testing the whole algorithms. @fingolfin, would that be appropriate? Are there further limits on RAM consumption?

I'm happy for any hint to beautify the code.

Thank you
Jordi

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. A bunch of quick comments added inline. Top level:

  • Please do not use tabs at all (see also https://docs.oscar-system.org/dev/DeveloperDocumentation/styleguide/).
  • adding multi minute / multi GB tests that are run every time is very problematic. Aren't there easier examples? They don't need to be "mathematically interesting" (interesting slow examples should be there, but not as part of the CI tests).

@@ -0,0 +1,1724 @@
using Oscar
Copy link
Member

Choose a reason for hiding this comment

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

This is not the way to go about it if you want to integrate this code into Oscar. Instead, add the code into a subdirectory of experimental -- please read https://docs.oscar-system.org/dev/Experimental/intro/ for details.

@@ -0,0 +1,1724 @@
using Oscar

global infoLevel = 0
Copy link
Member

Choose a reason for hiding this comment

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

A global like this is a no-go.

An alternative would be to use @vprint and friends in your code, see https://www.thofma.com/Hecke.jl/stable/features/macros/#verbosity-macros for details (and also you can grep through our code to see examples using it).

Comment on lines +10 to +16
groebnerwalk(
I::MPolyIdeal;
startOrder::MonomialOrdering = default_ordering(base_ring(I)),
targetOrder::Symbol = lex(base_ring(I)),
walktype::Symbol = :standard,
perturbationDegree::Int = 2,
)
Copy link
Member

Choose a reason for hiding this comment

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

Should be indented by four spaces, not tabs

walktype::Symbol = :standard,
perturbationDegree::Int = 2,
)
Compute a reduced Groebner basis w.r.t. to a monomial order by converting it using the Groebner Walk.
Copy link
Member

Choose a reason for hiding this comment

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

There must be an empty line between the function signatures and the text documenting it.

Comment on lines +20 to +25
Standard Walk (:standard) computes the Walk like it´s presented in Cox, Little & O´Shea (2005).
Generic Walk (:generic) computes the Walk like it´s presented in Fukuda, Jensen, Lauritzen & Thomas (2005).
Perturbed Walk (:perturbed, with p = degree of the perturbation) computes the Walk like it´s presented in Amrhein, Gloor & Küchlin (1997).
Tran´s Walk (:tran) computes the Walk like it´s presented in Tran (2000).
Fractal Walk (:fractalcombined) computes the Walk like it´s presented in Amrhein & Gloor (1998) with multiple extensions. The target monomial order has to be lex. This version uses the Buchberger Algorithm to skip weight vectors with entries bigger than Int32.
Fractal Walk (:fractal) computes the Walk like it´s presented in Amrhein & Gloor (1998). Pertubes only the target vector.
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
Standard Walk (:standard) computes the Walk like it´s presented in Cox, Little & O´Shea (2005).
Generic Walk (:generic) computes the Walk like it´s presented in Fukuda, Jensen, Lauritzen & Thomas (2005).
Perturbed Walk (:perturbed, with p = degree of the perturbation) computes the Walk like it´s presented in Amrhein, Gloor & Küchlin (1997).
Tran´s Walk (:tran) computes the Walk like it´s presented in Tran (2000).
Fractal Walk (:fractalcombined) computes the Walk like it´s presented in Amrhein & Gloor (1998) with multiple extensions. The target monomial order has to be lex. This version uses the Buchberger Algorithm to skip weight vectors with entries bigger than Int32.
Fractal Walk (:fractal) computes the Walk like it´s presented in Amrhein & Gloor (1998). Pertubes only the target vector.
Standard Walk (:standard) computes the Walk like it´s presented in Cox, Little & O´Shea (2005).
- Generic Walk (:generic) computes the Walk as presented in Fukuda, Jensen, Lauritzen & Thomas (2005).
- Perturbed Walk (:perturbed, with p = degree of the perturbation) computes the Walk ass presented in Amrhein, Gloor & Küchlin (1997).
- Tran's Walk (:tran) computes the Walk like as presented in Tran (2000).
- Fractal Walk (:fractalcombined) computes the Walk like as presented in Amrhein & Gloor (1998) with multiple extensions. The target monomial order has to be lex. This version uses the Buchberger algorithm to skip weight vectors with entries bigger than Int32.
- Fractal Walk (:fractal) computes the Walk as presented in Amrhein & Gloor (1998). Perturbs only the target vector.

Comment on lines +1694 to +1700
d0 = 0
for g in Singular.gens(G)
temp = deg(g, n)
if d0 < temp
d0 = temp
end
end
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
d0 = 0
for g in Singular.gens(G)
temp = deg(g, n)
if d0 < temp
d0 = temp
end
end
d0 = maximum(g -> deg(g, n), gens(g))

Comment on lines +1685 to +1693
M = 0
for i ∈ 1:n
for j ∈ 1:n
temp = T[i, j]
if M < temp
M = temp
end
end
end
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
M = 0
for i 1:n
for j 1:n
temp = T[i, j]
if M < temp
M = temp
end
end
end
M = maximum(T)

Comment on lines +1702 to +1705
w = d^(n - 1) * T[1, :]
for i ∈ 2:n
w = w + d^(n - i) * T[i, :]
end
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
w = d^(n - 1) * T[1, :]
for i 2:n
w = w + d^(n - i) * T[i, :]
end
w = sum(i -> d^(n - i) * T[i, :], 1:n)

Both versions are somewhat inefficient because they keep recomputing powers of d. Better would be something like this (could be slightly buggy but you should get the gist):

Suggested change
w = d^(n - 1) * T[1, :]
for i 2:n
w = w + d^(n - i) * T[i, :]
end
w = T[n, :]
e = one(d)
for i in 1:n-1
e *= d
w += e * T[n-i, :]
end

return true
end
return false
end
Copy link
Member

Choose a reason for hiding this comment

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

File should end in a newline

Suggested change
end
end

Comment on lines +1713 to +1718
if size(collect(Singular.coefficients(g)))[1] > 2
return true
end
if size(collect(Singular.coefficients(g)))[1] == 2
counter = counter + 1
end
Copy link
Member

Choose a reason for hiding this comment

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

Don't compute things twice

Suggested change
if size(collect(Singular.coefficients(g)))[1] > 2
return true
end
if size(collect(Singular.coefficients(g)))[1] == 2
counter = counter + 1
end
s = size(collect(Singular.coefficients(g)))[1]
s > 2 && return true
if s == 2
counter += 1
end

@fingolfin
Copy link
Member

@afkafkafk13 just explained that someone else in Berlin (@ooinaruhugh) is working on Gröbner walk stuff and is building on top of this to produce new code (they will probably open a new PR for that, but let's keep this one open for now as reference).

@fingolfin
Copy link
Member

Closing this in favor of PR #3821 which borrows some code from here but is under active development.

@fingolfin fingolfin closed this Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants