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

Keep variables for constant zero Polynomials #81

Merged
merged 5 commits into from
Apr 9, 2021
Merged

Keep variables for constant zero Polynomials #81

merged 5 commits into from
Apr 9, 2021

Conversation

manuelbb-upb
Copy link
Contributor

Fix of issue #80

Overwrite multconstant of MultivariatePolynomials so that in case of α==0 the variables of p are kept.
Else do the same as before (see operators.jl in MultivariatePolynomials).

@blegat
Copy link
Member

blegat commented Feb 3, 2021

Does this fix #79 as well ?

@manuelbb-upb
Copy link
Contributor Author

Does this fix #79 as well ?

No, it doesn't seem to.
But I can have a look into it in the near future.

@manuelbb-upb
Copy link
Contributor Author

It seems that #79 can be tackled by defining

function MA.mutable_operate_to!(p::Polynomial{true, T}, ::typeof(*), q1::Number, q2::MP.AbstractPolynomialLike) where T
    q1 * q2
end

This uses

function Base.:(*)(p::Polynomial, α)
    MP.multconstant(α,p);
end
Base.:(*)(α, p::Polynomial) = p * α;

but I am not familiar enough with the internals of the package to know whether the fix is any good.

src/mult.jl Outdated Show resolved Hide resolved
src/mult.jl Outdated Show resolved Hide resolved
@manuelbb-upb manuelbb-upb requested a review from blegat February 11, 2021 15:12
@manuelbb-upb manuelbb-upb marked this pull request as draft March 12, 2021 07:04
src/mult.jl Outdated Show resolved Hide resolved
src/mult.jl Outdated Show resolved Hide resolved
src/mult.jl Outdated
@@ -60,7 +73,7 @@ end
function _term_poly_mult(t::Term{C, S}, p::Polynomial{C, T}, op::Function) where {C, S, T}
U = MA.promote_operation(op, S, T)
if iszero(t)
zero(Polynomial{C, U})
zero( Polynomial{C,U} )
Copy link
Member

Choose a reason for hiding this comment

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

Remove spaces before and after parenthesis to stay consistent with the rest of the code base

src/mult.jl Outdated Show resolved Hide resolved
@blegat
Copy link
Member

blegat commented Mar 12, 2021

Thanks, looking good, could you add some tests ?

added tests from issue descriptions
file `mult.jl`, function `_multconstant_to!`
@manuelbb-upb
Copy link
Contributor Author

I have composed some basic tests from the issue descriptions. The nightly built still fails, but this seems to a separate problem.

Copy link
Member

@blegat blegat left a comment

Choose a reason for hiding this comment

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

Awesome, thanks a lot! Could your remove the Draft mode of the PR so that I can merge ?

@manuelbb-upb manuelbb-upb marked this pull request as ready for review March 16, 2021 14:42
@manuelbb-upb
Copy link
Contributor Author

Did so :) Thanks also for reviewing. This was the first time for me to actually try to contribute to another project and a great learning experience.

@blegat blegat merged commit 3c562c1 into JuliaAlgebra:master Apr 9, 2021
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