implement MA negation of a polynomial #285
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nsajko
added a commit
to nsajko/MultivariatePolynomials.jl
that referenced
this pull request
Nov 27, 2023
Performance improvements, support for more types. Still broken for `LinearAlgebra.Symmetric` polynomial matrices, producing a `MethodError` because of a missing `oneunit` method. This, however, seems like a separate matter that would better be addressed by a separate pull request. Performance comparison: ```julia-repl julia> versioninfo() Julia Version 1.11.0-DEV.972 Commit 9884e447e79 (2023-11-23 16:16 UTC) Build Info: Official https://julialang.org/ release Platform Info: OS: Linux (x86_64-linux-gnu) CPU: 8 × AMD Ryzen 3 5300U with Radeon Graphics WORD_SIZE: 64 LLVM: libLLVM-15.0.7 (ORCJIT, znver2) Threads: 11 on 8 virtual cores julia> using LinearAlgebra, DynamicPolynomials julia> function f(n) @PolyVar a b c d e diagm( -2 => fill(a, n - 2), -1 => fill(b, n - 1), 0 => fill(c, n), 2 => fill(e, n - 2), 1 => fill(d, n - 1), ) end f (generic function with 1 method) julia> const m15 = f(15); julia> const m16 = f(16); julia> @time det(m15); 1.945673 seconds (45.22 M allocations: 2.261 GiB, 20.60% gc time, 4.02% compilation time) julia> @time det(m15); 1.991062 seconds (45.22 M allocations: 2.261 GiB, 23.74% gc time) julia> @time det(m16); 4.596664 seconds (106.67 M allocations: 5.324 GiB, 22.65% gc time) julia> @time det(m16); 4.648503 seconds (106.67 M allocations: 5.324 GiB, 22.66% gc time) ``` The above REPL session is with this commit applied, and with all other recent PRs of mine applied, to MultivariatePolynomials.jl, DynamicPolynomials.jl, and MutableArithmetics.jl. The same computation with MultivariatePolynomials v0.5.3 ran for multiple minutes before I decided to just kill it. Depends on JuliaAlgebra#285. Fixes JuliaAlgebra#281.
blegat
reviewed
Nov 30, 2023
| @@ -264,6 +264,19 @@ end | |||
| Base.isapprox(p::_APL, α; kwargs...) = isapprox(promote(p, α)...; kwargs...) | |||
| Base.isapprox(α, p::_APL; kwargs...) = isapprox(promote(p, α)...; kwargs...) | |||
|
|
|||
| MA.operate!(::typeof(-), ::AbstractTermLike) = error("not implemented yet") | |||
|
|
|||
| MA.operate_to!(::AbstractTermLike, ::typeof(-), ::_APL) = error("not implemented yet") | |||
There was a problem hiding this comment.
Why don't we just keep the MethodError ?
There was a problem hiding this comment.
AbstractTermLike subtypes _APL, so we have to have a separate method. Do you want me to do throw(MethodError(...)) instead of error("not implemented yet")?
blegat
reviewed
Nov 30, 2023
|
|
||
| function MA.operate!(::typeof(-), p::_APL) | ||
| negate!! = x -> MA.operate!!(-, x) | ||
| return map_coefficients!(negate!!, p, nonzero = true) |
There was a problem hiding this comment.
You can use Base.Fix1(MA.operate!!, -) to avoid creating a closure that might allocate
There was a problem hiding this comment.
You can use
Base.Fix1(MA.operate!!, -)
I prefer to never use either Fix1 or Fix2, because I can never remember which is which. Should I?
to avoid creating a closure that might allocate
In this case this is not an issue, as the closure doesn't capture any variables.
|
Sounds good, can you just add tests ? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.