Named StateSpaceModel v2 – rebase and updates after #607#654
Named StateSpaceModel v2 – rebase and updates after #607#654opherdonchin wants to merge 2 commits intopymc-devs:mainfrom
Conversation
|
It looks like you checked in several files related to your local dev environment, could you clean it up? |
opherdonchin
force-pushed
the
named-statespace-v2
branch
from
9e5fd78 to
cbea84d
Compare
…nique graph naming Create a test to ensure multiple state space models can coexist with distinct names.
opherdonchin
force-pushed
the
named-statespace-v2
branch
from
cbea84d to
1bf3670
Compare
|
Sorry about that. The current PR includes only the changed files and a test: statespace.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #654 +/- ##
===========================================
+ Coverage 42.43% 69.24% +26.81%
===========================================
Files 63 73 +10
Lines 7207 7738 +531
===========================================
+ Hits 3058 5358 +2300
+ Misses 4149 2380 -1769
🚀 New features to boost your workflow:
|
jessegrabowski
left a comment
jessegrabowski
left a comment
There was a problem hiding this comment.
Nice, this is looking really clean. Just a few requests.
Also be sure to install pre-commit (pip install pre-commit; pre-commit install; pre-commit run --all) so your PR gets formatted and linted correctly.
| ) | ||
|
|
||
| if name in self._tensor_variable_info: | ||
| gname = self.graph_name(name) |
There was a problem hiding this comment.
As above. gname is too cryptic. Another option would be to change name to base_name and make name the prefixed version, since this will now be the "canonical" name that users see.
There was a problem hiding this comment.
Under the new scheme (base_name for the unprefixed name and name for the name in the graph) it makes most sense to just call this variable name.
| raise ValueError( | ||
| f"{name} is not a model parameter. All placeholder variables should correspond to model " | ||
| f"parameters." | ||
| f"{name} is not a model data-variable. All placeholder variables should correspond to model " |
There was a problem hiding this comment.
| f"{name} is not a model data-variable. All placeholder variables should correspond to model " | |
| f"{name} is not a model data variable. All placeholder variables should correspond to model " |
| f"{name} is not a model parameter. All placeholder variables should correspond to model " | ||
| f"parameters." | ||
| f"{name} is not a model data-variable. All placeholder variables should correspond to model " | ||
| f"data-variables." |
There was a problem hiding this comment.
| f"data-variables." | |
| f"data variables." |
| n_obs=self.ssm.k_endog, | ||
| obs_coords=obs_coords, | ||
| register_data=True, | ||
| data_name=self.graph_name("data"), |
There was a problem hiding this comment.
Let's let users choose the data name in the PyMCStateSpace constructor in this PR as well, instead of hard coding it to "data". The default can still be data, but then this line becomes data_name = self.graph_name(self.data_name)
tests/statespace/test_namespace.py
Outdated
| from pymc_extras.statespace.core.statespace import PyMCStateSpace | ||
|
|
||
|
|
||
| def test_two_statespace_models_can_coexist_with_names(monkeypatch): |
There was a problem hiding this comment.
put this test in the test_model file, it doesn't need a new file.
…d move namespace test
|
The new commit includes all changes suggested and the pre-commit processing. |
3 participants
Summary
This PR reintroduces the Named
StateSpaceModelchanges after rebasing ontomainfollowing the merge of #607.The previous version was submitted as #611. This update incorporates the changes introduced in #607 and resolves the resulting conflicts while preserving the intended named state space behavior.
No new conceptual changes are introduced beyond what was discussed in #611.
Changes
mainTests
Environment:
Results:
tests/statespace/corenutpie)tests/statespace/filterstest_kalman_filter_jax[cholesky])The single failing test occurs in the JAX Cholesky filter path and appears unrelated to the named model changes. All standard (non-JAX) filter paths and core state space tests pass.
Notes
main.