Named StateSpaceModel v2 – rebase and updates after #607#654
Named StateSpaceModel v2 – rebase and updates after #607#654opherdonchin wants to merge 3 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
2 times, most recently
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.
|
The new commit includes all changes suggested and the pre-commit processing. |
|
Really like where this landed, it's looking very clean. Could you double-check that we don't need to make any changes to Component or StructuralTimeSeries? Those are in the models/structural module, and are a sort-of parallel StatespaceModel that needs to be cleaned up some day. |
opherdonchin
force-pushed
the
named-statespace-v2
branch
2 times, most recently
from
ec6a4bd to
27a28ea
Compare
|
I'm working on the fix for structural models and in working on the Components create placeholders during I see a few options and I need help choosing:
Please let me know which direction you'd like to take it. |
|
We should be able to use You can check |
|
Any help from me needed here @opherdonchin @jessegrabowski ? |
…nique graph naming Create a test to ensure multiple state space models can coexist with distinct names.
…d move namespace test
StructuralTimeSeries.__init__ now uses pytensor.graph_replace to create fresh, prefixed placeholder variables in all SSM matrices when a model name is provided. This replaces the previous metadata-only rename approach, which left the actual graph placeholders unchanged and caused silent aliasing when the same Component instance was reused across multiple named StructuralTimeSeries models. Key changes: - Add _prefix_placeholder_variables() method that builds a replacement mapping from old to new (prefixed) placeholder variables, applies graph_replace across all SSM matrices, and rebuilds SymbolicVariableInfo and SymbolicDataInfo with aligned names. - Add _validate_symbolic_info() diagnostic helper. - Pass name=name through to PyMCStateSpace.__init__. - Add 8 focused tests covering the aliasing bug, name alignment, graph correctness, and the validation helper.
opherdonchin
force-pushed
the
named-statespace-v2
branch
from
27a28ea to
e950db5
Compare
|
@AlexAndorra: thanks for the nudge! I think what I've done makes sense and follows @jessegrabowski's suggestion. The branch is now rebased onto current main (v0.10.0) with a clean 3-commit history. What changed in the new commit:
I added appropriate tests in Let me know if this is a good fix and if anything else is needed before this is ready to merge? Thanks! |
4 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.