Skip to content

Conversation

AlexAndorra
Copy link
Contributor

  • TimeSeasonality:
    • Fixed dims and params_info for multivariate TS with innovations
    • Renamed parameter from coefs_ to params_ to be consistent with the other components
  • FrequencySeasonality:
    • Same thing
    • In addition, I fixed the coords of the parameters: the names of the unsaturated states were concatenated with the observed states, which was giving the wrong coords and shape

All of this is now fixed and tested (I also refactored the tests to use pytest fixtures more consistently).
Ready for review 🍾

@AlexAndorra AlexAndorra self-assigned this Aug 3, 2025
@AlexAndorra AlexAndorra added bug Something isn't working enhancements New feature or request labels Aug 3, 2025
@AlexAndorra
Copy link
Contributor Author

I fixed the tests that were failing because of the name change, but I'm a bit stumped by the other error. See this for instance:

FAILED tests/statespace/models/structural/test_against_statsmodels.py::test_structural_model_against_statsmodels[True-True-True-freq_seasonal2-stochastic_freq_seasonal2-12-True-3-True-True-False-True-False] 
- AssertionError: R expected to have shape (n_states, n_shocks), found (30, 17)
assert (30, 17) == (30, 6)

It looks like it's coming from _assert_coord_shapes_match_matrices, so it must be related to the coords changes in FrequencySeasonality, but I didn't figure it out yet. I'd be super grateful for some pointers to speed my understanding up if you have some @jessegrabowski 🙏

@jessegrabowski
Copy link
Member

I updated this PR. The issue that lead to the test failures was that frequency seasonality has a weird setup for innovations, that requires that every hidden state be present in the shocks.

I also changed the name order for the states to be fn_i_name (e.g. Sin_0_annual[data_1]) instead of fn_name_i (e.g. Sin_annual_0[data_1]). I thought this better adhered to our something_name decision.

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.

I fixed this up, can be merged after the CI passes

@AlexAndorra AlexAndorra merged commit 414017e into main Aug 4, 2025
17 checks passed
@AlexAndorra AlexAndorra deleted the fix-seasonality-dims branch August 4, 2025 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancements New feature or request statespace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants