Pass a string to NDArray(name=...) and raise NotImplemented on named models in SMC

[basnijholt]

Currently, name is a Model.

Thank your for opening a PR!

Depending on what your PR does, here are a few things you might want to address in the description:

• what are the (breaking) changes that this PR makes? → see thread & release notes
• important background, or details about the implementation → see thread
• are the changes—especially new features—covered by tests and docstrings?
• consider adding/updating relevant example notebooks
• right before it's ready to merge, mention the PR in the RELEASE-NOTES.md

[MarcoGorelli]

Nice, thanks @basnijholt ! Is it possible to add a little test somewhere, or to change an existing test so that this is checked?

[codecov]

Codecov Report

Merging #4365 (856a708) into master (1769258) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4365      +/-   ##
==========================================
- Coverage   88.00%   87.95%   -0.05%     
==========================================
  Files          88       88              
  Lines       14342    14493     +151     
==========================================
+ Hits        12622    12748     +126     
- Misses       1720     1745      +25     

Impacted Files	Coverage Δ
pymc3/smc/smc.py	99.52% <100.00%> (ø)
pymc3/distributions/timeseries.py	85.71% <0.00%> (-6.69%)	⬇️
pymc3/distributions/dist_math.py	91.97% <0.00%> (-4.11%)	⬇️
pymc3/distributions/continuous.py	93.31% <0.00%> (-1.23%)	⬇️
pymc3/distributions/bart.py	80.15% <0.00%> (-0.65%)	⬇️
pymc3/distributions/multivariate.py	82.92% <0.00%> (-0.63%)	⬇️
pymc3/distributions/mixture.py	88.69% <0.00%> (-0.16%)	⬇️
pymc3/math.py	68.64% <0.00%> (ø)
pymc3/sampling.py	89.62% <0.00%> (ø)
pymc3/sampling_jax.py	0.00% <0.00%> (ø)
... and 37 more

[basnijholt]

@MarcoGorelli, there is no public API that exposes this directly (AFAIK).

OTOH, these types of errors are easily caught by adding type hints and using mypy.

[twiecki]

A test would be good regardless, apparently this code-path wasn't tested before.

[basnijholt]

@twiecki, @MarcoGorelli, I have added a test.

[MarcoGorelli]

Thanks for adding a test!

Can you also add a note to the release notes?

[michaelosthege]

While making the test more precise, I found that the implementation does not actually support having a model name.

The reason is that the model name becomes a prefix to the variable name, which is also used as a kwarg in the line sim_data = self.function(**func_parameters). The function that is called is user-provided to the Simulator and does now have the prefix in the kwarg.

So these prefixes would have to be eliminated from the kwargs, but then the corner case of having nested models is hard to deal with. (yes, that's possible and no, nobody does that because it's very fragile)
With the upcoming changes towards RandomVariable Ops, we might get rid of the Model context entirely. Therefore I think we should not invest precious dev time into fixing this fragility.

I'm going to insert this before the NDArray creation:

if self.model.name:
    raise NotImplementedError(
        "The SMC implementation currently does not support named models. "
        "See https://github.com/pymc-devs/pymc3/pull/4365."
    )

[michaelosthege]

I just rebased to fix the README conflict. @aloctavodia if you approve, I think we can merge this.