add vi_kwargs to fit

[aloctavodia]

model.fit has an argument called method as well as pymc.fit to avoid those name clashing, this PR introduces a vi_kwargsargument that is passed to pymc.fit. As method="mcmc" is the most common method, we allow passing arguments as kwargs, instead of adding a mcmc_kwargs argument. This also add a missing test.

[codecov-commenter]

Codecov Report

Merging #553 (6788d6c) into main (236021f) will decrease coverage by 0.42%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #553      +/-   ##
==========================================
- Coverage   86.93%   86.50%   -0.43%     
==========================================
  Files          32       32              
  Lines        2655     2667      +12     
==========================================
- Hits         2308     2307       -1     
- Misses        347      360      +13     

Impacted Files	Coverage Δ
bambi/models.py	85.50% <ø> (-4.25%)	⬇️
bambi/backend/pymc.py	83.11% <100.00%> (+1.89%)	⬆️
bambi/tests/test_built_models.py	99.03% <100.00%> (+0.03%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[canyon289]

Nit: add exception somewhere above that if vi_kwargs is passed but method is not VI either a warning or exception is provided

Also nit: It feels like if theres vi kwargs, there should also be mcmc kwargs to mirror the API

[tomicapretto]

Agree with the first comment. Not so sure about the second. I understand that having vi_kwargs makes us think why not mcmc_kwargs as well. But it also makes me wonder whether we need mcmc_kwargs, vi_kwargs, and laplace_kwargs, which makes the signature much longer, or if it is better to replace method with a better name that does not overlap with parameters in pymc.fit() and keep a single **kwargs.

[aloctavodia]

As method="mcmc" is the most common method, is very convenient to allow for kwargs. Having mcmc_kwargs will turn super annoying. Instead I see vi (and laplace) as unusual methods so is ok if they deviate a little from that pattern. Having said that changing the method argument is an option. I just wanted to avoid breaking backward compatibility, but given that mcmc is the most common method probably that change will most likely unnoticed by most users.

[tomicapretto]

I agree that backwards compatibility may be a concern. Is it too overkill to have a deprecation warning now, saying we'll change the name of the method parameter, and clean up the signature later?

I propose inference_method as an alternative

[aloctavodia]

OK, let's do that