JumpProcesses.jl icon indicating copy to clipboard operation
JumpProcesses.jl copied to clipboard

aqua CI and related fixes

Open isaacsas opened this issue 1 year ago • 3 comments

Adds Aqua test, seems to indicate that https://github.com/SciML/JumpProcesses.jl/issues/384 is fixed.

@Vilin97 the Aqua test is giving a bunch of method ambiguities coming from the spatial mass action jump constructors. I think the issue is that you have overlapping definitions because of the union type you are using (i.e. multiple constructors cover the case that an input is typeNothing).

method_ambiguity = (kwcall(::NamedTuple, ::Type{SpatialMassActionJump}, srates::B, rs, ns, pmapper) where B<:Union{Nothing, AbstractMatrix} @ JumpProcesses ~/.julia/dev/JumpProcesses/src/spatial/spatial_massaction_jump.jl:61, kwcall(::NamedTuple, ::Type{SpatialMassActionJump}, urates::A, rs, ns, pmapper) where A<:Union{Nothing, AbstractVector} @ JumpProcesses ~/.julia/dev/JumpProcesses/src/spatial/spatial_massaction_jump.jl:73)
method_ambiguity = (SpatialMassActionJump(urates::A, srates::B, rs, ns; scale_rates, useiszero, nocopy) where {A<:Union{Nothing, AbstractVector}, B<:Union{Nothing, AbstractMatrix}} @ JumpProcesses ~/.julia/dev/JumpProcesses/src/spatial/spatial_massaction_jump.jl:53, SpatialMassActionJump(srates::B, rs, ns, pmapper; scale_rates, useiszero, nocopy) where B<:Union{Nothing, AbstractMatrix} @ JumpProcesses ~/.julia/dev/JumpProcesses/src/spatial/spatial_massaction_jump.jl:61)
method_ambiguity = (SpatialMassActionJump(srates::B, rs, ns; scale_rates, useiszero, nocopy) where B<:Union{Nothing, AbstractMatrix} @ JumpProcesses ~/.julia/dev/JumpProcesses/src/spatial/spatial_massaction_jump.jl:67, SpatialMassActionJump(urates::A, rs, ns; scale_rates, useiszero, nocopy) where A<:Union{Nothing, AbstractVector} @ JumpProcesses ~/.julia/dev/JumpProcesses/src/spatial/spatial_massaction_jump.jl:79)
method_ambiguity = (SpatialMassActionJump(srates::B, rs, ns, pmapper; scale_rates, useiszero, nocopy) where B<:Union{Nothing, AbstractMatrix} @ JumpProcesses ~/.julia/dev/JumpProcesses/src/spatial/spatial_massaction_jump.jl:61, SpatialMassActionJump(urates::A, rs, ns, pmapper; scale_rates, useiszero, nocopy) where A<:Union{Nothing, AbstractVector} @ JumpProcesses ~/.julia/dev/JumpProcesses/src/spatial/spatial_massaction_jump.jl:73)
method_ambiguity = (kwcall(::NamedTuple, ::Type{SpatialMassActionJump}, urates::A, srates::B, rs, ns) where {A<:Union{Nothing, AbstractVector}, B<:Union{Nothing, AbstractMatrix}} @ JumpProcesses ~/.julia/dev/JumpProcesses/src/spatial/spatial_massaction_jump.jl:53, kwcall(::NamedTuple, ::Type{SpatialMassActionJump}, srates::B, rs, ns, pmapper) where B<:Union{Nothing, AbstractMatrix} @ JumpProcesses ~/.julia/dev/JumpProcesses/src/spatial/spatial_massaction_jump.jl:61)
method_ambiguity = (kwcall(::NamedTuple, ::Type{SpatialMassActionJump}, srates::B, rs, ns) where B<:Union{Nothing, AbstractMatrix} @ JumpProcesses ~/.julia/dev/JumpProcesses/src/spatial/spatial_massaction_jump.jl:67, kwcall(::NamedTuple, ::Type{SpatialMassActionJump}, urates::A, rs, ns) where A<:Union{Nothing, AbstractVector} @ JumpProcesses ~/.julia/dev/JumpProcesses/src/spatial/spatial_massaction_jump.jl:79)
method_ambiguity = (JumpSet(vj, cj, rj, maj::MassActionJump{S, T, U, V}) where {S<:Number, T, U, V} @ JumpProcesses ~/.julia/dev/JumpProcesses/src/jumps.jl:497, JumpSet(jumps::JumpProcesses.AbstractJump...) @ JumpProcesses ~/.julia/dev/JumpProcesses/src/jumps.jl:512)
method_ambiguity = (rate_at_site(rx, site, smaj::SpatialMassActionJump{Nothing, B, S, U, V}) where {B, S, U, V} @ JumpProcesses ~/.julia/dev/JumpProcesses/src/spatial/spatial_massaction_jump.jl:112, rate_at_site(rx, site, smaj::SpatialMassActionJump{A, Nothing, S, U, V}) where {A, S, U, V} @ JumpProcesses ~/.julia/dev/JumpProcesses/src/spatial/spatial_massaction_jump.jl:116)

We can fix those in a separate PR, for now I just capped the method ambiguities at 8.

isaacsas avatar Jan 03 '24 19:01 isaacsas

@ChrisRackauckas do you have any idea why when testing locally on my Mac I only get 8 ambiguities but CI gets 35? Is this platform dependent?

isaacsas avatar Jan 03 '24 19:01 isaacsas

I fixed the ambiguities outside of the spatial solver with the exception of the ODE_DEFAULT_NORM one. I don't really understand why we are getting some of these ambiguities since they seem to often be arising from a version of the function defined on abstract types elsewhere, when we are specifying at least one concrete type in our method. I guess I don't quite understand dispatch rules in that case.

isaacsas avatar Jan 03 '24 22:01 isaacsas

I fixed the ambiguities coming from spatial stuff in https://github.com/SciML/JumpProcesses.jl/pull/392. Once this PR is merged, https://github.com/SciML/JumpProcesses.jl/pull/392 can be reviewed and merged as well.

Vilin97 avatar Jan 07 '24 08:01 Vilin97

Given the wonkiness of ambiguities reported by Aqua (i.e. being platform dependent for example), and its seemingly over aggressive declaration of things as ambiguous I'm going to close this PR. Some of the fixes might make sense to put in a followup PR, but I don't think we want to add Aqua to the test suite based on what I was observing in this PR.

Should SciML decide to add Aqua to CI across the board we can resurrect this.

isaacsas avatar Aug 06 '24 18:08 isaacsas

I'll keep the branch around so this can possibly be resurrected in the future.

isaacsas avatar Aug 06 '24 18:08 isaacsas