SciML / SciMLBase.jl

The Base interface of the SciML ecosystem
https://docs.sciml.ai/SciMLBase/stable
MIT License
121 stars 91 forks source link

Deprecate separate lb, ub arguments to IntegralProblem #576

Closed lxvm closed 6 months ago

lxvm commented 6 months ago

Fixes https://github.com/SciML/Integrals.jl/issues/198 Fixes https://github.com/SciML/Integrals.jl/issues/208

Specifically, this pr deprecates the following method:

@deprecate IntegralProblem(f::AbstractIntegralFunction,
    lb::Union{Number,AbstractVector{<:Number}},
    ub::Union{Number,AbstractVector{<:Number}},
    p = NullParameters(); kwargs...) IntegralProblem(f, (lb, ub), p; kwargs...)

@ChrisRackauckas this pr could also be a good time to decide on the promotion behavior for the endpoints of the integration domain. I actually think promotion should be handled at solve time since it could interfere with AD, e.g. using ForwardDiff and differentiating just the upper limit of integration.

Checklist

Additional context

Add any other context about the problem here.

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (416ca0a) 30.42% compared to head (4ca3c23) 27.18%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #576 +/- ## ========================================== - Coverage 30.42% 27.18% -3.24% ========================================== Files 53 53 Lines 4030 4021 -9 ========================================== - Hits 1226 1093 -133 - Misses 2804 2928 +124 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ChrisRackauckas commented 6 months ago

this pr could also be a good time to decide on the promotion behavior for the endpoints of the integration domain. I actually think promotion should be handled at solve time since it could interfere with AD, e.g. using ForwardDiff and differentiating just the upper limit of integration.

Yes that's how we do it with the other problem types so it makes sense to do the same thing on IntegralProblem.

ChrisRackauckas commented 6 months ago

I think this just needs to update the related tests and its good to go.