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

aqua CI and related fixes #389

Closed
wants to merge 6 commits into from
Closed

aqua CI and related fixes #389

wants to merge 6 commits into from

Conversation

isaacsas
Copy link
Member

@isaacsas isaacsas commented Jan 3, 2024

Adds Aqua test, seems to indicate that #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 isaacsas closed this Jan 3, 2024
@isaacsas isaacsas reopened this Jan 3, 2024
@isaacsas
Copy link
Member Author

isaacsas commented Jan 3, 2024

@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
Copy link
Member Author

isaacsas commented Jan 3, 2024

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.

@Vilin97
Copy link
Contributor

Vilin97 commented Jan 7, 2024

I fixed the ambiguities coming from spatial stuff in #392. Once this PR is merged, #392 can be reviewed and merged as well.

@isaacsas
Copy link
Member Author

isaacsas commented Aug 6, 2024

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 isaacsas closed this Aug 6, 2024
@isaacsas
Copy link
Member Author

isaacsas commented Aug 6, 2024

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

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.

3 participants