search

TuringLang / AdvancedHMC.jl

Robust, modular and efficient implementation of advanced Hamiltonian Monte Carlo algorithms
https://turinglang.org/AdvancedHMC.jl/
MIT License
228 stars 39 forks source link

feat: relativistic HMC #300

Closed xukai92 closed 1 year ago

xukai92 commented 1 year ago

Thanks for the review. I just recalled that we were intended to create a new branch for experimental stuff instead of merging it to the main branch. I will create a new PR for this instead and fix the CI.

yebai commented 1 year ago

There are pros and cons for maintaining two branches -- BUG fixes and improvements will take a lot longer to propagate to the main branch. We observed that trend on DynamicPPL.

It's ok to merge this PR to the master branch I think. The new algorithm is already separated from the stable code, which is the main reason for a new branch.

xukai92 commented 1 year ago

I’m a bit concerned about the new dependency though. It makes no sense for all upstream packages to depend on a package that is not well maintained (last update was like 2 years ago). I could make it optional using Revise.jo. How do you think?

torfjelde commented 1 year ago

IMO a different branch seems like the best option given the introduction of a new dep :confused:

xukai92 commented 1 year ago

@yebai I second to @torfjelde. Adding that dependency is a bad idea and managing it using Requires.jl is a bit cumbersome too. Since this is mostly for experimental features, I think we could just create a new branch called experimental, which we manually sync with the master. We just need to make sure we only introduce code change in the src/experimental folder; all other changes should come from merging with master.

yebai commented 1 year ago

@yebai I second to @torfjelde. Adding that dependency is a bad idea and managing it using Requires.jl is a bit cumbersome too. Since this is mostly for experimental features, I think we could just create a new branch called experimental, which we manually sync with the master. We just need to make sure we only introduce code change in the src/experimental folder; all other changes should come from merging with master.

Sure, that sounds good. Perhaps call that branch hmc-research instead?

EDIT: in that case, let's perhaps separate the changes to Kinect types into a new PR against master.

xukai92 commented 1 year ago

Sure, that sounds good. Perhaps call that branch hmc-research instead?

Sounds good. I will duplicate the current master and name it as hmc-research then create a new PR against it.

EDIT: in that case, let's perhaps separate the changes to Kinect types into a new PR against master.

Do you mean undo the already merged PR (#299) in master?