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

Move function wrappers into simplified interface #60

Closed
wants to merge 1 commit into from

Conversation

jgoldfar
Copy link
Contributor

@jgoldfar jgoldfar commented Feb 1, 2016

Move function wrappers into simplified interface and remove function argument (replace by closures) to fix tests on git master. Not sure if this is the "correct" way to do this compared to the comment on JuliaLang/julia#13412, but it does work.

Update: It works for simple cases, but not when the RHS is a closure.

…argument (replace by closures) to fix tests on git master
@ChrisRackauckas
Copy link
Member

Update: It works for simple cases, but not when the RHS is a closure.

Is that statement for v0.4? It should work on v0.5 since anonymous functions are generic in that case. But if closures aren't working with it in v0.5, I'd count that as a non-starter since I can point to a lot of examples where people are using closures to put in parameters. Anyways, this is not actionable unless tests pass. You should relabel this WIP if you plan on continuing this, or close.

@alyst was this touched in #67? I don't recall seeing changes to the function wrappers. Is this affected by the changes?

@alyst
Copy link
Contributor

alyst commented Sep 2, 2016

@ChrisRackauckas It was somewhat touched, in the sense that userdata parameter was declared to be of type Any directly by the wrapper generator, so simplified interface implementation had to be updated and now it should allow passing both Julian function and Julian user data.

I'm not sure we need to move the wrapper functions into the bodies of the simplified interface function.

Maybe it's worth investigating what is the overhead of the current wrapper implementation and whether there are ways to improve it (e.g. parameterize UserDataAndFunction by the type of user data and make it immutable). I left it out of the #67 scope.

CVODE/IDA allows specifying the other user functions, e.g. IDADlsSetDenseJacFn() (see ida_Roberts_dns.jl example). Currently they are not supported.

@ChrisRackauckas
Copy link
Member

Sounds like this is relevant to the discussion SciML/Roadmap#2

@jgoldfar
Copy link
Contributor Author

jgoldfar commented Sep 2, 2016

I've been following Julia development and using v0.5/master in my work for a while, but haven't touched the Sundials portion for a bit. I believe the issue with ccallable closures was present in v0.4 and has been (at least partially) resolved recently; the change in this PR fixed some issues on the then-current master, but not all.

@jgoldfar
Copy link
Contributor Author

jgoldfar commented Sep 2, 2016

See also JuliaLang/julia#1096

@ChrisRackauckas
Copy link
Member

Was it not resolved by JuliaLang/julia#13412 now that anonymous functions are all generic?

@jgoldfar
Copy link
Contributor Author

jgoldfar commented Sep 2, 2016

No: the issue I was trying to work around was caused by that; Ref{Function} no longer made sense. Ptr{Void} does though.

@ChrisRackauckas
Copy link
Member

I see.

@ChrisRackauckas
Copy link
Member

No longer relevant.

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.

None yet

3 participants