Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #142 +/- ##
===========================================
- Coverage 72.30% 46.90% -25.41%
===========================================
Files 19 19
Lines 1914 1936 +22
===========================================
- Hits 1384 908 -476
- Misses 530 1028 +498 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@OriolAbril I have some big open questions in the PR description if you have time to take a look. Its almost ready to go once I get those questions answered |
OriolAbril
left a comment
OriolAbril
left a comment
There was a problem hiding this comment.
- Proposal: Use NumPyroInferenceAdapter Pattern vs. Separate NumPyroConverter for Each Inference Method
The adapter sounds good. It is also a somewhat similar pattern to what we use to interface with computational and plotting backends
2 & 3: nested_sampler and adapter or not
I think it makes sense to remove it, especially if it is easyish to get it to work in another way. Moreover, from the looks of it you'll be doing a large chunk of the maintenance on that so your opinion also gets extra weight.
We do not need to maintain backwards compatibility so having a single from_numpyro that can take MCMC or the custom adapter class sounds good, simplified code sounds even better (and a big reason for the refactor to begin with).
I think the main reason for having both things would be visibility but the conversion guide at the top level of the sidebar I think will be enough if it shows the different patterns including use of custom adapter.
Alright made these changes and tests are passing!
I'd say this PR is more "extensible to future changes" as opposed to "simplified". I think the one thing that really adds code complexity is supporting 14 different input arguments, where many different combinations can be provided/withheld. If we really want simplicity in the codebase, I wonder if the numpyro converter should be focused more on converting posteriors and the rest of the inputs could be provided by a user via Curious if you have any thoughts or think I should take another pass at thinking of ways to simplify? |
| Constant data used for out-of-sample predictions. | ||
| log_likelihood : bool, default False | ||
| Whether to compute and include log likelihood in the output. | ||
| index_origin : int, optional |
There was a problem hiding this comment.
could potentially get rid of index_origin if we want rcParams to handle it. it currently defaults to rcParams when not provided. Emcee and Pystan converters dont use these, but cmdstanpy does.
I'm leaning towards leaving it to introduce less changes - Thoughts?
src/arviz_base/io_numpyro.py
Outdated
| # For MCMC from numpyro, we need to reshape the sample shape | ||
| # based on the number of chains provided | ||
| if self.nchains is not None: | ||
| ndraws = aelem.shape[0] // self.nchains |
There was a problem hiding this comment.
this feels dangerous (its the previous logic we used, I just moved it out of init)
its for cases where a posterior isnt provided and numpyro has a flattened prior/predictions/posterior_predictive dictionary
I'm going to try and rethink this
| import lazy_loader as _lazy | ||
|
|
||
|
|
||
| if TYPE_CHECKING: |
There was a problem hiding this comment.
this seemed to be the pattern necessary to get lazy_loader working when there is both lazy loading and regular module loading (I had trouble finding other patterns).
Most packages choose to go full lazy loading, but I wanted to keep the scoping of the PR to just numpyro, so I chose this partial lazy loading pattern
|
@OriolAbril ready for review! Apologies for the large notebook diff, I updated the conversion guide and stripped the output so that future changes wont have large diffs |
3 participants
Addresses: #106
Background
Refactors Numpyro Converters to use an Inference Object Adapter - this should be more extensible long term and makes it easier to support new numpyro inference methods (such as Nested Sampling)
Changes
SVIWrapperwas renamed toSVIAdapterfrom_numpyrobackwards compatibility is broken in some cases, such as when a posterior isn't providedNumPyroInferenceAdapterabstract base class to standardize NumPyro inference objectssample_dims,sample_shape,get_samples(),get_extra_fields()MCMCAdapterfor MCMC inference with consistent interfaceNumPyroConverterworks uniformly with all adapter typeslog_likelihoodparameter default fromNonetoFalsein allfrom_numpyro*functions for clarityOpen Questions (with resolution notes)
1) Proposal: Use
NumPyroInferenceAdapterPattern vs. SeparateNumPyroConverterfor Each Inference MethodOne design question is whether we should use the
NumPyroInferenceAdapterpattern or instead create a separateNumPyroConvertersubclass for each inference method.While having both an Adapter class and a Converter class might seem awkward, the Adapter pattern offers several advantages:
My Opinion: I preferred this adapter pattern to the abstract base converter approach in my other draft pr
Resolution Note: We went with the adapter pattern
2) Proposal: Remove
from_nested_sampler()(experimental, contrib module in numpyro) in favor of a genericfrom_numpyro_adapter()?Currently,
from_nested_sampler()would support numpyro'sNestedSampler, which lives in the contrib module and is marked as experimental. Given its experimental status, it may not be worth adding explicit support in ArviZ.Instead, I'm wondering if we should adopt a generic
from_numpyro_adapter()pattern:NumPyroInferenceAdapter(e.g.,NestedSamplerAdapter).from_numpyro_adapter()function.Advantages of this approach:
My Opinion: I think I should probably remove the nested sampler implementation since its experiment and in numpyro's contrib module. The
from_numpyro_adapter()method gives users the flexibility to support it on their end pretty easilyResolution Note: We removed the nested sampler
3) Question: Should we fold
from_numpyro_adapterintofrom_numpyro()? Do we need to maintain backwards compatibility?Problem: When a
posteriorisn't provided, we still needsample_dimsto reshape input prior/posterior predictive data (e.g., for MCMC chains). Addingsample_dimsas an argument tofrom_numpyro()would break backwards compatibility. To avoid this, the current implementation uses separatefrom_numpyro*()functions, which can infersample_dimsautomatically for each inference type.Alternative (breaks backwards compatibility, simplifies code):
from_numpyro()function to support all inference objects with an optionalsample_dimsargument.posteriorcan beMCMC(default) or anyNumPyroInferenceAdapter.posteriorisNone,sample_dimsmust be provided, otherwise it fails.from_numpyro_adapter()entirely.from_numpyro_svifor ease of useCompromise for backwards compatibility:
sample_dims = ["chain", "draw"]if not provided, but issue a warning.My Opinion: I'm unsure which approach is best here, open to feedback!
Resolution Note: We removed from_numpyro_adapter in favor of from_numpyro working on either MCMC or the adapter classes