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 normalize_with_delta #339

Merged
merged 2 commits into from
Mar 16, 2021

Conversation

tthsqe12
Copy link
Contributor

No description provided.

@thofma
Copy link
Collaborator

thofma commented Mar 16, 2021

The keyword argument names (equidimDec and primeDec) look a bit funny in the Oscar world, but I guess this is Singular legacy?

Also, according to codecov there are some untested branches.

@fingolfin
Copy link
Member

There is something wrong with coverage generated by jldoctest, see issue #323.

@thofma
Copy link
Collaborator

thofma commented Mar 16, 2021

There is no jldoctest added here, just normal tests I think.

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.

OK by me, but there are oddities in the test, which @tthsqe12 hopefully can double check?

end

@doc Markdown.doc"""

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
normalize_with_delta(A::MPolyQuo; alg=:equidimDec)

for algorithm in (:equidimDec, :primeDec)
R, (x, y, z) = PolynomialRing(QQ, ["x", "y", "z"])
Q, _ = quo(R, ideal(R, [(x^2 - y^3)*(x^2 + y^2)*x]))
for (S, M, FI) in normalize(Q)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this was meant? (I can't test it right now)

Suggested change
for (S, M, FI) in normalize(Q)
for (S, M, FI) in normalize(Q; alg=algorithm)

@test parent(M(Q(x+y))) == S
end

stuff, deltas, tot_delta = normalize_with_delta(Q)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this?

Suggested change
stuff, deltas, tot_delta = normalize_with_delta(Q)
stuff, deltas, tot_delta = normalize_with_delta(Q; alg=algorithm)

@tthsqe12
Copy link
Contributor Author

Sorry all, It should be better now.

@fieker
Copy link
Contributor

fieker commented Mar 16, 2021

I'll merge it after the tests...

@fieker fieker merged commit 93cdde4 into oscar-system:master Mar 16, 2021
@tthsqe12 tthsqe12 deleted the add_normalize_with_delta branch July 19, 2021 09:01
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.

None yet

4 participants