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

Hilbert series integration #2698

Merged
merged 81 commits into from
Aug 24, 2023

Conversation

JohnAAbbott
Copy link
Contributor

Minor revisions (see also discussion #2674)

JohnAAbbott and others added 30 commits August 7, 2023 11:53
src/Rings/hilbert.jl Outdated Show resolved Hide resolved
Comment on lines 941 to 944
catch _
# solve_non_negative must have thrown because there is a non-zero soln
error("given weights permit infinite dimensional homogeneous spaces")
end
Copy link
Collaborator

@thofma thofma Aug 18, 2023

Choose a reason for hiding this comment

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

The catched error should be checked against the "expected" error and rethrown otherwise.

(The function could also throw, because your input is garbage or the interface changed or ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected! Thanks for pointing that out. Anyway that code is obsolescent - hopefully is_positively_graded is better written.

src/Rings/hilbert.jl Outdated Show resolved Hide resolved
src/Rings/hilbert.jl Outdated Show resolved Hide resolved
@fingolfin
Copy link
Member

@HechtiDerLachs (and perhaps also @wdecker ) can you have a look at this? Shall we merge it (perhaps up to the minor tweaks I suggested)?

@fingolfin
Copy link
Member

And just to clarify: does this resolve issue #2674 ?

@wdecker
Copy link
Collaborator

wdecker commented Aug 23, 2023

@fingolfin The crucial part is marked # !!!OBSOLESCENT!!! in Oscar.jl/src/Rings/hilbert.jl. This should be removed and be replaced by the already updated is_positiely_graded function wherever it is called.
@HechtiDerLachs Can you please do this? In particular, you know better than me how this is handled in Oscar.jl/src/Modules/hilbert.jl

@HechtiDerLachs
Copy link
Collaborator

I still get the following:

x, y) = graded_polynomial_ring(QQ, ["x", "y"], [1, -1])
(Graded multivariate polynomial ring in 2 variables over QQ, MPolyDecRingElem{QQFieldElem, QQMPolyRingElem}[x, y])

julia> is_positively_graded(R)
true

That's not what's supposed to happen, is it?

Should I really try to fix something already?

@HechtiDerLachs
Copy link
Collaborator

Currently tests fail. Probably due to the bug described above.

@wdecker
Copy link
Collaborator

wdecker commented Aug 23, 2023

I will take a look

@wdecker
Copy link
Collaborator

wdecker commented Aug 23, 2023

@HechtiDerLachs There is an unmotivated break in John's is_positive_grading_matrix function. Do you agree?

@HechtiDerLachs
Copy link
Collaborator

No, I think if the comment prior to that method is correct, then the break is fine. Earlier zero entries are caught by the continue and trailing entries can be skipped.

@wdecker
Copy link
Collaborator

wdecker commented Aug 23, 2023

Ooops, the whole is_positive_grading_matrix function is scrap. We have to rewrite it in a seperate PR.

@wdecker
Copy link
Collaborator

wdecker commented Aug 23, 2023

Sorry @JohnAAbbott, the mistake is on my side, your matrix is the transpose of mine. Things will be changed correspondingly.

Copy link
Collaborator

@HechtiDerLachs HechtiDerLachs left a comment

Choose a reason for hiding this comment

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

Looks fine from my side now.

src/Rings/mpoly-affine-algebras.jl Outdated Show resolved Hide resolved
Co-authored-by: Matthias Zach <85350711+HechtiDerLachs@users.noreply.github.com>
@fingolfin fingolfin merged commit b3682ec into oscar-system:master Aug 24, 2023
8 of 12 checks passed
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.

5 participants