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

[WIP] Refreshing the wrappers for Sundials 6.6 #415

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ViralBShah
Copy link
Contributor

@ViralBShah ViralBShah commented Jul 13, 2023

This PR now has the newly generated C wrappers for Sundials 6.5. The package needs to be updated to match the changes in the wrappers.

There's some new ARKRhsFn_wrapper thing that needs to be dealt with (among other things). But at least the clang generators are now running through.

cc @ChrisRackauckas

@ViralBShah ViralBShah changed the title Refreshing the wrappers for Sundials 6 Refreshing the wrappers for Sundials 6.5 Jul 15, 2023
Use Julia 1.10 and clang 0.14
Update to use SuiteSparse 7
@ViralBShah ViralBShah changed the title Refreshing the wrappers for Sundials 6.5 Refreshing the wrappers for Sundials 6.6 Aug 22, 2023
@ChrisRackauckas
Copy link
Member

Why is >v1.6 required?

@ViralBShah
Copy link
Contributor Author

Never mind. I think this is generating for Sundials 5.2 still. I think I just need Julia 1.10 for generating the headers with the latest Clang.jl. Let me see if I can get it working first and then will worry about bumping julia_compat down to 1.6, which should just work.

@ViralBShah
Copy link
Contributor Author

This is the error running Clang. The clang wrappers need updating:

ERROR: LoadError: BoundsError: attempt to access 2-element Vector{Any} at index [3]
Stacktrace:
  [1] getindex(A::Vector{Any}, i1::Int64)
    @ Base ./essentials.jl:13
  [2] wrap_sundials_api(expr::Expr)
    @ Main ~/.julia/dev/Sundials/gen/generate.jl:93
  [3] rewrite(ex::Expr)

@ViralBShah ViralBShah marked this pull request as draft August 22, 2023 21:40
@sjdaines
Copy link
Contributor

Minimum change that I think would be needed to the generated code in libsundials_api.jl to make the NVector memory management work as in the current release (ie as it does following fixes in release v4.12.0 #380 ):

The generated ccall wrappers need to take NVector arguments and pass through to ccall so that ccall then uses the cconvert and unsafe_convert machinery to preserve the lifetime of the NVector (which may well be a temporary generated by the calling code).

So eg the generated wrapper

function CVodeInit(cvode_mem, f::CVRhsFn, t0::realtype, y0::N_Vector)
    ccall((:CVodeInit, libsundials_cvodes), Cint, (Ptr{Cvoid}, CVRhsFn, realtype, N_Vector), cvode_mem, f, t0, y0)
end

would need to be instead:

function CVodeInit(cvode_mem, f::CVRhsFn, t0::realtype, y0::NVector)
    ccall((:CVodeInit, libsundials_cvodes), Cint, (Ptr{Cvoid}, CVRhsFn, realtype, N_Vector), cvode_mem, f, t0, y0)
end

I'm guessing it might also be necessary to accept other types for y0 as in the current released code:

  • to avoid extra conversions where the caller already has an N_Vector and is responsible for managing the lifetime, allow
function CVodeInit(cvode_mem, f::CVRhsFn, t0::realtype, y0::Union{NVector, N_Vector})
  • to allow y0::Vector , provide
function CVodeInit(cvode_mem, f::CVRhsFn, t0::realtype, y0)
    return CVodeInit(cvode_mem, f, t0, convert(NVector, y0))
end

(or perhaps this is overcomplicated and just removing the type signature to vector arguments (y0 etc) would be enough to let ccall apply whatever is needed ? Not sure if it would chain together convert and then cconvert and unsafe_convert to handle a Julia Vector though ?)

johnomotani added a commit to mabarnes/moment_kinetics that referenced this pull request Sep 5, 2023
…ssive"

These settings are only available in sundials-6.2, which is not yet
supported by Sundials.jl (see
SciML/Sundials.jl#415).
johnomotani added a commit to mabarnes/moment_kinetics that referenced this pull request Sep 5, 2023
…ssive"

These settings are only available in sundials-6.2, which is not yet
supported by Sundials.jl (see
SciML/Sundials.jl#415).
johnomotani added a commit to mabarnes/moment_kinetics that referenced this pull request Sep 5, 2023
…ssive"

These settings are only available in sundials-6.2, which is not yet
supported by Sundials.jl (see
SciML/Sundials.jl#415).
johnomotani added a commit to mabarnes/moment_kinetics that referenced this pull request Sep 5, 2023
…ssive"

These settings are only available in sundials-6.2, which is not yet
supported by Sundials.jl (see
SciML/Sundials.jl#415).
johnomotani added a commit to mabarnes/moment_kinetics that referenced this pull request Sep 6, 2023
…ssive"

These settings are only available in sundials-6.2, which is not yet
supported by Sundials.jl (see
SciML/Sundials.jl#415).
johnomotani added a commit to mabarnes/moment_kinetics that referenced this pull request Sep 10, 2023
…ssive"

These settings are only available in sundials-6.2, which is not yet
supported by Sundials.jl (see
SciML/Sundials.jl#415).
@oscardssmith
Copy link
Contributor

Can this be rebased on master to see if it works now?

@ViralBShah ViralBShah closed this Feb 7, 2024
@ViralBShah ViralBShah reopened this Feb 7, 2024
@ViralBShah ViralBShah marked this pull request as ready for review February 7, 2024 14:59
@ViralBShah ViralBShah changed the title Refreshing the wrappers for Sundials 6.6 [WIP] Refreshing the wrappers for Sundials 6.6 Feb 7, 2024
@ViralBShah
Copy link
Contributor Author

ViralBShah commented Feb 7, 2024

I thought this PR needs a substantial amount of work. I really doubt it will work that easily.

@ViralBShah ViralBShah marked this pull request as draft February 17, 2024 16:40
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.

4 participants