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

small adjustions #384

Merged
merged 4 commits into from
Mar 26, 2021
Merged

small adjustions #384

merged 4 commits into from
Mar 26, 2021

Conversation

wdecker
Copy link
Collaborator

@wdecker wdecker commented Mar 25, 2021

No description provided.

"""
function hilbert_series(A::MPolyQuo)
function hilbert_series(A::U) where U <: Union{MPolyRing{T}, MPolyQuo{T}} where T
Copy link
Contributor

Choose a reason for hiding this comment

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

The Union can be put directly in lieu of U, and T is not used so can be removed:

Suggested change
function hilbert_series(A::U) where U <: Union{MPolyRing{T}, MPolyQuo{T}} where T
function hilbert_series(A::Union{MPolyRing, MPolyQuo})

@doc Markdown.doc"""
hilbert_series_reduced(A::MPolyQuo)
hilbert_series_reduced(A::U) where U <: Union{MPolyRing{T}, MPolyQuo{T}} where T
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
hilbert_series_reduced(A::U) where U <: Union{MPolyRing{T}, MPolyQuo{T}} where T
hilbert_series_reduced(A::Union{MPolyRing, MPolyQuo})

@rfourquet
Copy link
Contributor

The test error says:

MPolyQuo.normalize: Error During Test at /home/runner/work/Oscar.jl/Oscar.jl/test/Rings/MPolyQuo.jl:18
  Got exception outside of a @test
  UndefVarError: normalize not defined

@wdecker
Copy link
Collaborator Author

wdecker commented Mar 25, 2021

O.k., I replaced normalize by normalization and normalize_with_delta by normalization_with_delta without knowing that someone (I guess Dan) had already added tests. Sorry, I should have checked this. Also, the command is no longer in MPolyQuo.jl. but in mpoly_affine_algebras.jl. Should I now adjust the test file accordingly?

@rfourquet
Copy link
Contributor

Should I now adjust the test file accordingly?

Yes this should be enough, the test occurences of normalize or normalize_with_delta seem to be only in the "test/Rings/MPolyQuo.jl".

Comment on lines +43 to 50
function hilbert_series(A::Union{MPolyRing, MPolyQuo})
if typeof(A) == FmpqMPolyRing
Zt, t = ZZ["t"]
return (one(parent(t)), (1-t)^(ngens(A)))
end
H = HilbertData(A.I)
return hilbert_series(H,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.

@wdecker so I don't want to annoy you, and I am fine with merging this, but let me point out that what you do here is "not the Julia way". There, we should have two methods, one for each type. This allows Julia to produce better code, and is simply more in line with usual conventions there. So, in this example, it'd look like this (assuming I understand everything correctly here):

Suggested change
function hilbert_series(A::Union{MPolyRing, MPolyQuo})
if typeof(A) == FmpqMPolyRing
Zt, t = ZZ["t"]
return (one(parent(t)), (1-t)^(ngens(A)))
end
H = HilbertData(A.I)
return hilbert_series(H,1)
end
function hilbert_series(A::FmpqMPolyRing)
Zt, t = ZZ["t"]
return (one(parent(t)), (1-t)^(ngens(A)))
end
function hilbert_series(A::Union{MPolyRing, MPolyQuo})
H = HilbertData(A.I)
return hilbert_series(H,1)
end

This assumes you really wanted to just treat FmpqMPolyRing in a special way, but not e.g. FmpzMPolyRing or NmodMPolyRing or .... So perhaps you also meant this?

Suggested change
function hilbert_series(A::Union{MPolyRing, MPolyQuo})
if typeof(A) == FmpqMPolyRing
Zt, t = ZZ["t"]
return (one(parent(t)), (1-t)^(ngens(A)))
end
H = HilbertData(A.I)
return hilbert_series(H,1)
end
function hilbert_series(A:: MPolyRing)
Zt, t = ZZ["t"]
return (one(parent(t)), (1-t)^(ngens(A)))
end
function hilbert_series(A::MPolyQuo)
H = HilbertData(A.I)
return hilbert_series(H,1)
end

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Of course I meant MPolyRing rather than FmpqMPolyRing, this was a copy and paste mistake. Thanks for catching it. With regard to using Union{MPolyRing, MPolyQuo}: I will come back to this later later in the more general context of graded modules. For the time being, I would appreciate if someone could merge this so that I can continue working, building on new work by Delphine and Hans. I am greatful for all the comments which help me to get a better understanding of what I do, but waiting and waiting and waiting for a merge is really annoying.

@fingolfin fingolfin merged commit a5c5f81 into master Mar 26, 2021
@fingolfin fingolfin deleted the Wolfram branch March 26, 2021 16:35
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