Skip to content

==(α, q::MP.RationalPoly) and ==(p::MP.AbstractPolynomialLike, α) cause many invalidations #290

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

Open
sumiya11 opened this issue Feb 14, 2024 · 5 comments

Comments

@sumiya11
Copy link
Contributor

To check this, I run

# from https://sciml.ai/news/2022/09/21/compile_time/
using SnoopCompile

invalidations = @snoopr begin
    using AbstractAlgebra
    R, (x,y) = polynomial_ring(QQ, ["x","y"], ordering=:degrevlex)
    using Groebner
    groebner([x^2 - y^2, x*y^2 + x])
end

trees = SnoopCompile.invalidation_trees(invalidations);

@show length(SnoopCompile.uinvalidated(invalidations)) # show total invalidations (1102)

show(trees[end-1])
show(trees[end-2])

(Groebner.jl loads MultivariatePolynomials.jl).

I have

 inserting ==(α, q::MultivariatePolynomials.RationalPoly) @ MultivariatePolynomials C:\Users\User\.julia\packages\MultivariatePolynomials\TpRhf\src\comparison.jl:118 invalidated:
   backedges: 1: superseding ==(x, y) @ Base Base.jl:165 with MethodInstance for ==(::Core.MethodInstance, ::Any) (6 children)
              2: superseding ==(x, y) @ Base Base.jl:165 with MethodInstance for ==(::Module, ::Any) (11 children)
              3: superseding ==(x, y) @ Base Base.jl:165 with MethodInstance for ==(::Method, ::Any) (33 children)
              4: superseding ==(x, y) @ Base Base.jl:165 with MethodInstance for ==(::Symbol, ::Any) (154 children)
              5: superseding ==(x, y) @ Base Base.jl:165 with MethodInstance for ==(::Core.TypeName, ::Any) (164 children)
              6: superseding ==(x, y) @ Base Base.jl:165 with MethodInstance for ==(::Symbol, ::Any) (1 children)
              7: superseding ==(x, y) @ Base Base.jl:165 with MethodInstance for ==(::Symbol, ::Any) (4 children)

 inserting ==(p::MultivariatePolynomials.AbstractPolynomialLike, α) @ MultivariatePolynomials C:\Users\User\.julia\packages\MultivariatePolynomials\TpRhf\src\operators.jl:65 invalidated:
   backedges: 1: superseding ==(x, y) @ Base Base.jl:165 with MethodInstance for ==(::Any, ::Cthulhu.DInfo.DebugInfo) (6 children)
              2: superseding ==(x, y) @ Base Base.jl:165 with MethodInstance for ==(::Any, ::Core.MethodInstance) (8 children)
              3: superseding ==(x, y) @ Base Base.jl:165 with MethodInstance for ==(::Any, ::Symbol) (420 children)
              4: superseding ==(x, y) @ Base Base.jl:165 with MethodInstance for ==(::Any, ::typeof(display)) (2 children)
              5: superseding ==(x, y) @ Base Base.jl:165 with MethodInstance for ==(::Any, ::Symbol) (1 children)
              6: superseding ==(x, y) @ Base Base.jl:165 with MethodInstance for ==(::Any, ::Symbol) (1 children)
              7: superseding ==(x, y) @ Base Base.jl:165 with MethodInstance for ==(::Any, ::typeof(display)) (1 children)
              8: superseding ==(x, y) @ Base Base.jl:165 with MethodInstance for ==(::Any, ::typeof(display)) (11 children)
              9: superseding ==(x, y) @ Base Base.jl:165 with MethodInstance for ==(::Any, ::Symbol) (2 children)

This amounts to 825 method invalidations out of 1102.

I wonder if it would be okay to rewrite these methods with the use of isa (or even remove them) ?

@serenity4
Copy link

serenity4 commented Apr 2, 2025

I am not familiar enough with this package to answer that, but if AbstractPolynomialLike and RationalPoly were to subtype Number, then by default they would hit the ==(x::Number, y::Number) = ==(promote(x, y)...) fallback so they could use promote to avoid having to redefine == for Any arguments.

@StefanKarpinski
Copy link

What about just changing Any to Union{Number, AbstractPolynomialLike, RationalPoly}?

@blegat
Copy link
Member

blegat commented Apr 2, 2025

AbstractPolynomialLike does not subtype Number for the same reason JuMP.AbstractJuMPScalar does not : because it does not satisfy many of the assumptions that are usually satisfied by Number, see jump-dev/MutableArithmetics.jl#47
We aim to support as coefficient Any an not Number because we want to support coefficients that are AbstractArray or JuMP.AbstractJuMPScalar as well.
The issue of ==(a, b) = ==(promote(a, b)...) is that this promotion would allocate while for JuMP expressions and polynomials, if you are comparing with an object of a simpler structure, it's cheaper to check if the more complex structure is equal to the simpler one than allocating the more complex one for the simpler object.
@sumiya11 what is your suggestion using isa ?

@blegat
Copy link
Member

blegat commented Apr 3, 2025

If this is causing a lot of issues, I can change Any to Union{Number,MutableArithmetics.AbstractMutable,AbstractArray} which I think would cover most use cases.

@sumiya11
Copy link
Contributor Author

sumiya11 commented Apr 5, 2025

@sumiya11 what is your suggestion using isa ?

Hum...
I am afraid I don't recall what I had in mind back then.

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

No branches or pull requests

4 participants