convert to immutable for CUDA tests #151
Conversation
|
Tests are failing due to this methoderror. I'm not sure why given the constructor in the new
|
Co-authored-by: Christopher Rackauckas <[email protected]>
src/SimpleNonlinearSolve.jl
Outdated
| function SciMLBase.solve(prob::Union{ImmutableNonlinearProblem}, alg::AbstractSimpleNonlinearSolveAlgorithm, | ||
| args...; sensealg = nothing, u0 = nothing, p = nothing, kwargs...) |
There was a problem hiding this comment.
The conversion should go here.
There was a problem hiding this comment.
I moved the conversion to inside the solve in the last commit. Now I'm getting a method error when precompiling due to the startup workload. It is complaining about this line when solving a problem that has been converted and its not clear to me how to fix it:
|
I will review it in detail later, but tagging @Vaibhavdixit02 because it will very likely break GPUPSO. Maybe we tag a breaking release just for safety |
|
Since that package wasn't released, you should be fine on that end. |
|
GPUPSO should be fine if this is done right? We just need to make sure the kernels use everything immutable. |
src/immutable_nonlinear_problem.jl
Outdated
| staticarray_itize(x) = x | ||
| staticarray_itize(x::Vector) = SVector{length(x)}(x) | ||
| staticarray_itize(x::SizedVector) = SVector{length(x)}(x) | ||
| staticarray_itize(x::Matrix) = SMatrix{size(x)...}(x) | ||
| staticarray_itize(x::SizedMatrix) = SMatrix{size(x)...}(x) | ||
|
|
||
| function Base.convert(::Type{ImmutableNonlinearProblem}, prob::T) where {T <: NonlinearProblem} | ||
| ImmutableNonlinearProblem{isinplace(prob)}(prob.f, | ||
| staticarray_itize(prob.u0), | ||
| staticarray_itize(prob.p), |
There was a problem hiding this comment.
This isn't going to be type stable, we shouldn't copy this part. Don't automatically make things static array, just immutable the problem.
src/nlsolve/broyden.jl
Outdated
| @@ -22,7 +22,7 @@ end | |||
|
|
|||
| __get_linesearch(::SimpleBroyden{LS}) where {LS} = Val(LS) | |||
|
|
|||
| function SciMLBase.__solve(prob::NonlinearProblem, alg::SimpleBroyden, args...; | |||
| function SciMLBase.__solve(prob::Union{NonlinearProblem, ImmutableNonlinearProblem}, alg::SimpleBroyden, args...; | |||
There was a problem hiding this comment.
None of these unions should be required. If done correctly, the top level should change to an ImmutableNonlinearProblem, and then they should all be that.
|
Left a few comments. If those two things are done this should be quick to finish. |
|
I made changes according to your comments. The simple adjoint test is failing with this error: Any thoughts on the fix for that? |
|
These tests are also failing: https://github.com/SciML/SimpleNonlinearSolve.jl/blob/40958d2c2fdff0986d113ea1f5ed1ffa96316e89/test/gpu/cuda_tests.jl#L40C1-L40C107 Doesn't the |
|
They kernel function needs to be the immutable form. And this would need an FAQ entry.
Add the immutable stuff to the AD dispatches? |
Converts NonlinearProblem to ImmutableNonlinearProblem for CUDA compatibility (SciML/NonlinearSolve.jl#439)