Skip to content

Conversation

@Dekermanjian
Copy link
Contributor

This is a draft proposal for #598

The idea is to handle each component separately using _set_{component} methods and all information are stored using data classes for easy mapping.

I believe this will simplify our tests of these components and will reduce redundancies where we have the same information spread across multiple sub-components like data_names and data_info.

@jessegrabowski let me know what you think I put a little notebook together to showcase the changes.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@Dekermanjian Dekermanjian marked this pull request as draft November 2, 2025 17:50
Copy link
Member

@jessegrabowski jessegrabowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great first pass, much cleaner than what we have now.

@jessegrabowski
Copy link
Member

We can also keep all of the existing properties like state_names, shock_names, state_dims, etc, but move them to the base class and just extract the requested info from the relevant Info objects.

@jessegrabowski jessegrabowski changed the title proposal for updating propogate_component_properties using data classes Represent statespace metadata with dataclasses Nov 7, 2025
@jessegrabowski
Copy link
Member

Reflecting on it, I am convinced this is the way to go. It's 1000x more ergonomic. I made some changes to your initial code to make the API more "dictionary like", and to reduce code duplication. I moved everything to statespace/core/properties.py, because this is ultimately going to replace what we have in both the core models and the components.

@Dekermanjian
Copy link
Contributor Author

@jessegrabowski, this is looking really cool! What can I do to help push this forward?

@jessegrabowski
Copy link
Member

jessegrabowski commented Nov 7, 2025

Delete the new regression_dataclass.py and simply refactor regression.py to use the new stuff.

We should keep your notebook with the plan to add it as a new example for the docs. Or it can be merged into the custom statespace notebook. So that should also be updated to import from the new properties.py file

@Dekermanjian
Copy link
Contributor Author

Perfect! I'll work on that today!! It is really looking cool!

@Dekermanjian
Copy link
Contributor Author

@jessegrabowski, I agree with all of your comments above. I am going to start making those changes.

…uplicate with warning

2. removed unnecessary imports from __init__ after deleting regression_dataclass
3. updated components and structural classes to only utilize dataclasses and pull other objects from <foo>_info dataclasses
4. updated tests to conform to dataclass api
2. created tests for add and merge methods
3. added utility to convert from snake to pascal and integrated it in error messaging
Copy link
Member

@jessegrabowski jessegrabowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete review, I'll continue tomorrow AM

Comment on lines +47 to +48
# if key in index:
# raise ValueError(f"Duplicate {self.key_field} '{key}' detected.") # This needs to be possible for shared states
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That shouldn't happen here though, it should come up in merge or add right? And we handle it there with the allow_duplicates flag

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what happens is because our data classes are immutable the __post_init__ runs right after our merge/add because we always return new objects of the same dataclass and it see that there are duplicate keys even though the merge/add method had allowed them via allow_duplicates.

raise AttributeError(f"Items missing attribute '{self.key_field}': {missing_attr}")
object.__setattr__(self, "_index", index)

def _key(self, item: T) -> str:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look like we are using this at the moment. I think it could come in handle later on to check for specific attributes.

self.state_names = list(state_names) if state_names is not None else []
self.observed_state_names = (
list(observed_state_names) if observed_state_names is not None else []
self.param_info = ParameterInfo(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change the component signature to just take the Info objects directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be nice. Right now, there is this intermediate conversion step that needs to take place.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok let's leave it for the next PR then and make the priority here just getting each model to be represented in the new way

…_duplicates is False

2. converted component attributes into properties
3. removed _combine_property method
4. removed redundant observed_states property
5. fixed indentation bug
@Dekermanjian
Copy link
Contributor Author

Hey @jessegrabowski, by switching a lot of the component attributes to properties I was able to simplify a good amount of downstream methods. If you don't mind taking a look at the current state of this before I go ahead and do the same with the rest of the SSM components.


self.coords_info = CoordInfo(coords=[regression_state_coord, endogenous_state_coord])

def populate_component_properties(self) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method won't be unique to regression right? We will want to move it up to the base class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jessegrabowski, in the base class there is a populate_component_properties method that raises a NotImplemented. Did you want to replace that with a generic method that sets _set_<foo> for the 2 defaults (shocks and states) that we provide?

self.make_symbolic_graph()

self._component_info = {
self._component_info = { # Should this be a dataclass??
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave it for now though. The mission creep on this PR is already pretty terrible.

raise NotImplementedError

def _set_shocks(self) -> None:
def _set_shocks(self) -> ShockInfo:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make a comment in each of these methods that these are generic defaults that can/should be replaced with component specific logic. I got momentarily confused because they are called _set but they actually return info (i.e. they do a get, not a set). The name makes sense in the context of populate_component_properties`, but not in a vacuum; hence the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that is right. These shouldn't be doing a get they should actually be doing a set. I will fix it and put in a docstring with the explanation that these are defaults that should be replaced.

@jessegrabowski
Copy link
Member

Yeah it looks really great! Go ahead and do the others. Excited to get this over the finish line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants