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

discuss Langevin #5

Open
paciorek opened this issue Jun 16, 2022 · 7 comments
Open

discuss Langevin #5

paciorek opened this issue Jun 16, 2022 · 7 comments
Labels
question Further information is requested

Comments

@paciorek
Copy link
Contributor

I had done some work a while back to implement a version of Langevin (in nimble branch extend_langevin) that adapts the full covariance following a discussion with Giacomo Zanella (https://arxiv.org/abs/1908.11812). I hadn't gotten acceptable performance with that or (I believe) with the basic Langevin on my test cases and hadn't gotten a chance to get back to it to explore further.

I'm wary of exposing the current Langevin in nimbleHMC until we do more exploration of performance/behavior. One thought is that we remove it entirely for now, while another is that we don't export it.

@danielturek @perrydv, thoughts?

@paciorek paciorek added the question Further information is requested label Jun 16, 2022
@danielturek
Copy link
Member

@paciorek I'd be fine with the non-export option.

Also noting that the nimbleHMC repo README says:

  • Langevin sampler (under development)

@paciorek
Copy link
Contributor Author

Great, I'll leave it to you to do that.

@danielturek
Copy link
Member

sampler_langevin is no longer exported in the nimbleHMC package.

I'm leaving this issue open, for discussion about re-working / improving the langevin sampler.

@paciorek
Copy link
Contributor Author

Revisiting this in light of upcoming release.

sampler_langevin is not exported but there is roxygen and one does see it if one does library(help=nimbleHMC). I'm wary of this. I think my vote at this point would be to remove the roxygen for it and make sure it doesn't show up in man.

@danielturek how does that sound?

@danielturek
Copy link
Member

@paciorek Yes, that sounds reasonable to me. I'm happy to make this change (remove Roxygen for sampler_langevin) if we're in agreement here.

@paciorek
Copy link
Contributor Author

'removal' could also simply be "commenting out" the roxygen "comments" by adding another level of comment hashes, so as to keep the info in the code file.

@danielturek
Copy link
Member

@paciorek Yes absolutely, that was my intention. I'll do this right now. Keep this issue open, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants