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

Add hilbert_series #694

Merged
merged 10 commits into from
Aug 15, 2023
Merged

Add hilbert_series #694

merged 10 commits into from
Aug 15, 2023

Conversation

hannes14
Copy link
Member

@hannes14 hannes14 commented Aug 7, 2023

hilbert_series(I, Qt::PolyRing) wih the new Hilbert function algorithm for sideal and smodule

@hannes14 hannes14 closed this Aug 11, 2023
@hannes14 hannes14 reopened this Aug 11, 2023
return h;
});
Singular.method("scHilbPolyWeighted", [](ideal I, ring r, jlcxx::ArrayRef<int> weights, ring Qt) {
intvec *w = to_intvec(weights);
Copy link
Contributor

@JohnAAbbott JohnAAbbott Aug 14, 2023

Choose a reason for hiding this comment

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

Assumes that weights has the correct size, and that all its entries are positive (and maybe not too big?)
This is apparently checked in line 1304 below

function hilbert_series(I::sideal{spoly{T}}, w::Vector{<:Integer}, Qt::PolyRing) where T <: Nemo.FieldElem
I.isGB || error("Not a Groebner basis")
R = base_ring(I)
length(w) == nvars(R) || error("wrong number of weights")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it useful in the error mesg to say how many weights were received, and how many were expected?

is the Hilbert-Poincare series of $I$ for weights $(1, \dots, 1)$.
"""
function hilbert_series(I::sideal{spoly{T}}, Qt::PolyRing) where T <: Nemo.FieldElem
I.isGB || error("Not a Groebner basis")
Copy link
Contributor

Choose a reason for hiding this comment

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

If it is the caller's responsibility to ensure that GB for I is available then this should be in the doc!
Maybe consider an optional/kw arg which the user can use to permit hilbert_series to compute a GB if necessary?

Copy link
Contributor

@JohnAAbbott JohnAAbbott left a comment

Choose a reason for hiding this comment

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

I have put some comments/suggestions in the code deltas

@hannes14
Copy link
Member Author

addressed suggestions with 068c4a8

@@ -686,6 +686,11 @@ end

@test ngens(std_hilbert(j, h, w, complete_reduction = true)) ==
ngens(std(j, complete_reduction = true))
Qt,(t)= polynomial_ring(QQ, ["t"])
t=gen(Qt,1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the assignment t = gen(Qt,1) superfluous? I think line 698 already assigns the correct value to t, right?

Copy link
Contributor

@JohnAAbbott JohnAAbbott left a comment

Choose a reason for hiding this comment

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

Not sure how I can actually try running this code; anyway, reading the code makes me believe that it is correct (under wholly reasonably assumptions about how Oscar-Singular interface works). I believe line 690 in sideal-test.jl is superfluous (but harmless), and for me in an interactive Oscar session the syntax in line 691 produces an error (but works when Ideal is replaced by ideal (all lowercase)).

@hannes14
Copy link
Member Author

Inconsistencies between Singular.jl and Oscar: Singular.jl has 7 variants of Ideal(..), but no ideal(..).
See for example the constructions in test/ideal/sideal-test.jl

Copy link
Contributor

@JohnAAbbott JohnAAbbott left a comment

Choose a reason for hiding this comment

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

Approved: Hans has explained the parts which were unclear to me

Copy link
Contributor

@JohnAAbbott JohnAAbbott left a comment

Choose a reason for hiding this comment

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

Thanks: neat workaround for one of julia's "foibles"

Copy link
Contributor

@JohnAAbbott JohnAAbbott left a comment

Choose a reason for hiding this comment

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

Approved

Comment on lines 689 to 690
Qt,_= polynomial_ring(QQ, ["t"])
t=gen(Qt,1)
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
Qt,_= polynomial_ring(QQ, ["t"])
t=gen(Qt,1)
Qt,t= polynomial_ring(QQ, ["t"])

deps/src/ideals.cpp Outdated Show resolved Hide resolved
deps/src/ideals.cpp Outdated Show resolved Hide resolved
@fingolfin fingolfin changed the title Hs/hilb Add hilbert_series Aug 15, 2023
@fingolfin fingolfin merged commit d0bae9a into oscar-system:master Aug 15, 2023
8 of 9 checks passed
@hannes14 hannes14 deleted the hs/hilb branch August 15, 2023 12:49
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.

3 participants