Closed GStechschulte closed 7 months ago
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
Note https://github.com/jax-ml/bayeux/blob/main/bayeux/_src/shared.py#L115 is what I use in bayeux
, though I do a fair amount of manual work cleaning things up, or removing arguments that are supplied elsewhere.
If you run the slightly modified
def get_default_signature(fn):
defaults = {}
for key, val in inspect.signature(fn).parameters.items():
if val.default is not inspect.Signature.empty:
defaults[key] = val.default
return defaults
on pm.sample
, you get the pleasant
{'draws': 1000,
'tune': 1000,
'chains': None,
'cores': None,
'random_seed': None,
'progressbar': True,
'step': None,
'nuts_sampler': 'pymc',
'initvals': None,
'init': 'auto',
'jitter_max_retries': 10,
'n_init': 200000,
'trace': None,
'discard_tuned_samples': True,
'compute_convergence_checks': True,
'keep_warning_stat': False,
'return_inferencedata': True,
'idata_kwargs': None,
'nuts_sampler_kwargs': None,
'callback': None,
'mp_ctx': None,
'model': None}
As far as I know the signature for pm.sample()
has arguments for many different things. Maybe we can hard-code the subset of parameters we want to query from it and only report those?
I am not so sure tests should be added for this? The .get_kwargs
method already raises an error if the user passes an inference method that is not in the list of available methods.
Then, for bmb.inference_methods.name
I suppose a test could be added to assert specific key names (mcmc
, vi
) exist in the dict?
@GStechschulte I see what you mean. I don't have a strong opinion here. The only thing I can add is that if we leave it untested it'll decrease coverage. I know high coverage does not mean our test suite is perfect, but I do think that in general lower coverage is worse. We could omit the inference_methods.py
module from coverage but I'm not sure if it is a good thing or not.
Another option would be to merge as it is and open an issue so someone tests this in the future (as it's not critical).
Many thanks for the review @ColCarroll and @tomicapretto
I added a small test to check that the keys (mcmc
, vi
) exist when calling bmb.inference_methods.names
. As well as a test to ensure that a ValueError
is raised if a user passes an unsupported inference method name.
Closes #791. This PR adds a convenient class
InferenceMethods
that allows users to access the available inference methods and kwargs.For example, the inference methods
and the default kwargs for a given inference method
Additionally, this convenience class is now imported and used in
backend/pymc.py
to obtain the bayeux and pymc inference methods. I have updated relevant doc strings and the alternative samplers notebook as well.