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

typos and Aqua CI #276

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

typos and Aqua CI #276

wants to merge 2 commits into from

Conversation

ArnoStrouwen
Copy link
Member

method ambiguity detection maybe can't cope with these weird types?

julia> DelayDiffEq.SciMLBase.SciMLSolution
Union{SciMLSolution, DESolution} (alias for Union{SciMLBase.AbstractEnsembleSolution, SciMLBase.AbstractNoTimeSolution, SciMLBase.AbstractNoiseProcess, SciMLBase.AbstractTimeseriesSolution})

julia> DelayDiffEq.SciMLBase.DESolution
Union{SciMLSolution, DESolution} (alias for Union{SciMLBase.AbstractEnsembleSolution, SciMLBase.AbstractNoTimeSolution, SciMLBase.AbstractNoiseProcess, SciMLBase.AbstractTimeseriesSolution})

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

No, I don't think DESolution etc. should be treated as owned by DelayDiffEq. It's just type piracy:

DelayDiffEq.jl/src/utils.jl

Lines 219 to 233 in 3e9c423

function Base.sizehint!(sol::DESolution, n)
sizehint!(sol.u, n)
sizehint!(sol.t, n)
sizehint!(sol.k, n)
nothing
end
"""
sizehint!(sol::DESolution, alg, tspan, tstops, saveat; kwargs...)
Suggest that solution `sol` reserves capacity for a number of elements that
depends on the parameter settings of the numerical solver.
"""
function Base.sizehint!(sol::DESolution, alg, tspan, tstops, saveat;

Comment on lines +24 to +25
LinearAlgebra = "1"
Logging = "1"
Copy link
Member

Choose a reason for hiding this comment

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

There's an unfortunate problem (https://discourse.julialang.org/t/psa-compat-requirements-in-the-general-registry-are-changing/104958#update-november-9th-2023-2) that means it is generally safer to use

Suggested change
LinearAlgebra = "1"
Logging = "1"
LinearAlgebra = "<0.0.1, 1"
Logging = "<0.0.1, 1"

OrdinaryDiffEq = "6.49.1"
Printf = "1"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Printf = "1"
Printf = "<0.0.1, 1"

Comment on lines +3 to +12
Aqua.find_persistent_tasks_deps(DelayDiffEq)
Aqua.test_ambiguities(DelayDiffEq, recursive = false)
Aqua.test_deps_compat(DelayDiffEq)
Aqua.test_piracies(DelayDiffEq,
treat_as_own = [DelayDiffEq.SciMLBase.DESolution,
DelayDiffEq.SciMLBase.SciMLSolution])
Aqua.test_project_extras(DelayDiffEq)
Aqua.test_stale_deps(DelayDiffEq)
Aqua.test_unbound_args(DelayDiffEq)
Aqua.test_undefined_exports(DelayDiffEq)
Copy link
Member

Choose a reason for hiding this comment

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

Again, I'd prefer to use Aqua.test_all to not miss out on any tests in the future (again, it seems there's some fine-tuning with keyword arguments necessary anyway).
Since the ambiguity check is problematic currently, I suggest (as e.g. in Distributions and Pumas) using

Suggested change
Aqua.find_persistent_tasks_deps(DelayDiffEq)
Aqua.test_ambiguities(DelayDiffEq, recursive = false)
Aqua.test_deps_compat(DelayDiffEq)
Aqua.test_piracies(DelayDiffEq,
treat_as_own = [DelayDiffEq.SciMLBase.DESolution,
DelayDiffEq.SciMLBase.SciMLSolution])
Aqua.test_project_extras(DelayDiffEq)
Aqua.test_stale_deps(DelayDiffEq)
Aqua.test_unbound_args(DelayDiffEq)
Aqua.test_undefined_exports(DelayDiffEq)
# Test ambiguities separately without Base and Core
# Ref: https://github.com/JuliaTesting/Aqua.jl/issues/77
Aqua.test_all(DelayDiffEq; ambiguities = false)
Aqua.test_ambiguities(DelayDiffEq)

@ChrisRackauckas
Copy link
Member

Oh I didn't know there was a sizehint! overload here. Yeah no that should move to SciMLBase.

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