Move kernel functions to their algorithm-class folder.

[albcab]

Closes #492 before people start building new algorithms with the previous structure.

Besides loosing the aliases, the other major breaking change is that for adaptation algorithms the user can no longer pass a class as the algorithm to adapt with. Hence, the user now needs to pass a string with the algorithm_name for the adaptation kernel to invoke the kernel and init method. This applies to window_adaptation and pathfinder_adaptation.

[codecov]

Codecov Report

Merging #501 (3077fa6) into main (65c01a8) will increase coverage by 0.01%.
The diff coverage is 99.35%.

@@            Coverage Diff             @@
##             main     #501      +/-   ##
==========================================
+ Coverage   99.28%   99.29%   +0.01%     
==========================================
  Files          47       47              
  Lines        1948     1978      +30     
==========================================
+ Hits         1934     1964      +30     
  Misses         14       14              

Impacted Files	Coverage Δ
blackjax/vi/pathfinder.py	97.01% <92.30%> (-1.17%)	⬇️
blackjax/vi/meanfield_vi.py	96.77% <92.85%> (-1.19%)	⬇️
blackjax/__init__.py	100.00% <100.00%> (ø)
blackjax/adaptation/base.py	100.00% <100.00%> (ø)
blackjax/adaptation/meads_adaptation.py	100.00% <100.00%> (ø)
blackjax/adaptation/pathfinder_adaptation.py	100.00% <100.00%> (ø)
blackjax/adaptation/window_adaptation.py	100.00% <100.00%> (ø)
blackjax/mcmc/elliptical_slice.py	95.77% <100.00%> (+0.77%)	⬆️
blackjax/mcmc/ghmc.py	100.00% <100.00%> (ø)
blackjax/mcmc/hmc.py	100.00% <100.00%> (ø)
... and 10 more

[albcab]

not sure what is up with the building of the docs by readthedocs

[junpenglao]

Love the overall direction this is going, some high level comments:

1. we now have things like blackjax.mcmc.hmc.hmc for high level API, instead of blackjax.hmc (alias of blackjax.kernel.hmc), should we think of some ways to make this a bit better on the eye? For example renaming the file names to something more explicit (hamiltonian_monte_carlo.py instead of hmc.py)
2. window_adaptation is taking string as input, but it might be unnecessary restrictive (a user can wrap the random walk MH sampler with a step size and inverse mass matrix parameter and window_adaptation would work

[junpenglao]

Not sure I understand this change, seems not needed?

[albcab]

Because blackjax.hmc (or any other kernel constructor) is now a function, it creates an object with a specific location in memory when evaluated. If this object is created outside the test function test_univariate_normal then calls to the chex variant function inference_loop have a different call sign for each of the devices/cores (because each call sign has a different memory location), throwing an error when collecting tests. The error looks like ERROR collecting gw1. Different tests were collected between gw0 and gw1.

[junpenglao]

IIUC, in the top level __init__.py you imported the symbol, so you dont need to make this change

[rlouf]

not sure what is up with the building of the docs by readthedocs

Fixed this temporarily by allowing the build to pass even if there are warnings (we'll need to figure these out later) in #502.

[rlouf]

Hence, the user now needs to pass a string with the algorithm_name for the adaptation kernel to invoke the kernel and init method. This applies to window_adaptation and pathfinder_adaptation.

Ok, so that's why I actually added these classes :/ Not a big fan of passing names as strings.

[junpenglao]

Ok, so that's why I actually added these classes :/ Not a big fan of passing names as strings.

Similar issue for when we are writing test: #501 (comment)

[albcab]

1. we now have things like blackjax.mcmc.hmc.hmc for high level API, instead of blackjax.hmc (alias of blackjax.kernel.hmc), should we think of some ways to make this a bit better on the eye? For example renaming the file names to something more explicit (hamiltonian_monte_carlo.py instead of hmc.py)

We still have blackjax.hmc. What we don't have is blackjax.hmc.kernel and blackjax.hmc.init, instead we have blackjax.mcmc.hmc.kernel and blackjax.mcmc.hmc.init.

2. window_adaptation is taking string as input, but it might be unnecessary restrictive (a user can wrap the random walk MH sampler with a step size and inverse mass matrix parameter and window_adaptation would work

Not a big fan of passing names as strings.

Agree with both. Our other option is to have the user pass both the init and kernel function directly (without aliases though).

[albcab]

are we turning our backs to Marx and staying on a class structure?

Python does seem to be, inherently, an object oriented language...

Similar issue for when we are writing test: #501 (comment)

[albcab]

Back to classes (but still have the function commits if we want to go back to that), also I've made naming on sampling algorithms consistent #280

[junpenglao]

Thanks for working on this.
Since we need to keep the class implementation for high level API, the devil's advocate is that keeping them in the same file (like what we have originally) makes the implementation pattern clear and easy to reference / compare.

[albcab]

Also to consider, the kernel.py file will grow to unsustainable levels and I think it's easier to give general guidelines if new algorithms are implemented in one file (assuming al basic components of the method are already implemented).

[albcab]

@junpenglao this file is a good example of various algorithms in one script. Since both additive_step_random_walk and irmh are special cases of rmh it makes sense to keep them all together, wdyt?

[junpenglao]

LGTM.

Only one minor comment: any reason why you are putting the top level API for blackjax.adaptation before def base(...)?

[albcab]

Only one minor comment: any reason why you are putting the top level API for blackjax.adaptation before def base(...)?

Didn't notice this, probably best to keep the set up consistent...