-
Notifications
You must be signed in to change notification settings - Fork 12
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
Necessary steps for inclusion in JuliaDiff ? #6
Comments
Just reading through the documentation now. This looks really great! Some comments:
This will certainly be a very useful package. I'd be happy to include it in JuliaDiff once the above comments are addressed. I'd also prefer to wait until the latest version is tagged and passes the |
I would also recommend using https://github.com/IainNZ/Coverage.jl. Especially for tricky symbolic simplification rules, it's important to make sure that all code paths are tested. |
Thanks for letting me know about the progress you made @fredo-dedup, it sounds really exciting that you cover higher order derivatives - does this mean we can get third order derivatives easily using your package? If so, this will allow running all our MCMC samplers using autodiff. |
Sorry for being quiet but as usual I have trouble freeing time to work on this. Yes It is supposed to handle any order of derivation. The trouble is that there is a bug appearing at order 3 that I haven't solved yet (for vector input only, a single scalar seems to work, see example in doc). Note that the generated code size will grow quickly if there is no simplifications found at each step. Run time will also grow exponentially O(1) for first order, then O(n) for second order, O(n^2) for third, etc... It is all based on reverse accumulation that is re-applied to each array element of the previous order derivative. The Function input version simply extracts the uncompressed AST and runs the standard expression derivation. The generated expression is then evaluated to a function (which need not be anonymous if that can make a difference). Going forward I need to find the 3rd order bug, adapt MCMC, and tag both ReverseDiffSource and MCMC. I am a bit reluctant to upgrade MCMC though since there was a talk about separating the DSL from the core MCMC sampling in this package and this would make that adaptation useless. How about creating MCMCTask and MCMCDSL (only a suggestion of name) and when working ditching MCMC ? As for tidying up the package, I have the Travis build passing, a 91% coverage and a (simple) doc so I don't think there is too much left undone there (PkgEval should pass too since the Travis buils passes, no ?) |
Yes, I agree with the divide-and-conquer approach. In fact, I remember we had a long conversation a couple of months ago and I had expressed some thoughts by creating this diagram. I have been doing some work on MCMCAPI, MCMCTasks and MCMCModels, so we have to somehow coordinate our efforts @fredo-dedup so that we work in parallel rather than orthogonally... For me, the most important of the above 3 gits is MCMCAPI, as I had made some changes in comparison to MCMC. I am not sure how we will coordinate, I am open to suggestions :) |
The most important change I have made is perhaps here: https://github.com/scidom/MCMCAPI.jl/blob/master/src/samplers.jl As you will see, I allow to pass a proposal function to the sampler (which is restricted to be a normal in our existing implementation). |
I can't really comment on how to structure the MCMC package, but maybe to work around this issue you could put a maximum version limit in MCMC's
I agree that the package is ready in this sense. 91% coverage is certainly sufficient. If you're planning on bumping the package soon, we can move it over to JuliaDiff whenever you're ready. I would just prefer that there's not a long delay, to avoid the case where users will try to install the package and not get the version that matches the documentation. |
Any update on this? Looking forward to have this package as part of JuliaDiff. |
@fredo-dedup why don't you go ahead with the migration of ReverseDiffSource to JuliaDiff? The MCMC package is another story and shouldn't hinder accommodating your AD package under the JuliaDiff umbrella. On a separate note, I have made a lot of progress on our MCMC code - I should be able to submit a PR to MCMC in at most two weeks' time. |
I did some improvements (coverage, doc) in the devl branch. Will try first
|
Hi @scidom @mlubin Sorry for the delay. Now tell if I am missing something for inclusion in JuliaDiff. Thanks. |
Looks good to me! @fredo-dedup, I've given you temporary permission to transfer the repository into JuliaDiff. You should be able to do it through the settings of this repo. |
It looks great to me too! Once the transfer to JuliaDiff is complete we need to update the url in metadata. |
Transfer is done. |
Cool! Feel free to announce the release on julia-users, julia-opt, julia-stats, etc as you see appropriate. |
Quick question @fredo-dedup, did you fix manage to fix the bug for third-order AD on vectors? Apparently this is not urgent, I am asking only out of interest. |
No it is not fixed. My plan is to tackle it by first rewriting that part of the code because I let it grown too much and it has become too complex. I'll open an issue to make the situation clear. |
Thanks for the update @fredo-dedup. It would be great if you manage to fix this, because we can then run PMALA and RMHMC in MCMC using AD. I will be able to pay attention to your AD work and familiarize myself with it in 2-3 weeks from now, as I am currently working on a fork of our MCMC package to complete a major upgrade. |
Thanks for your help in the transfer. |
Hi @mlubin , @scidom
I have just committed to master (without tagging so as not break the MCMC package) the new version of ReverseDiffSource I have been working on in the past months. Its internals are completely rewritten, it is now using a custom graph that allows more things than the previous approach.
The biggest changes are that it now accepts
for
loops and an unlimited derivation order (except for a nasty bug appearing sometimes for order >= 3). There is also an experimental support for Function input (instead of Expression input).I have also added a documentation.
Could you tell me if it is ready for inclusion in the JuliaDiff group as we talked about last may/june ?
I'd be happy to hear your opinion !
Thanks.
The text was updated successfully, but these errors were encountered: