Skip to content

Add duration parameter in TimeSeasonality #509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
Jul 21, 2025

Conversation

gdeleva
Copy link
Contributor

@gdeleva gdeleva commented Jun 9, 2025

This PR adds one new parameter to TimeSeasonality: duration.
This determines how long each seasonal period is held constant before moving to the next.

Solves #483

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.

Awesome start! You have exactly the right idea. I think we can be a bit more clever with the math is all.

For the actual implementation, I don't think we can do anything clever -- we'll need to expand the transition matrix to s * d and repeat the rows (and also repeat the starting values). The replicates should all be from the same symbolic value, so the user still just provides s inputs.

@gdeleva gdeleva marked this pull request as ready for review July 15, 2025 13:55
@gdeleva
Copy link
Contributor Author

gdeleva commented Jul 15, 2025

@jessegrabowski This PR may be ready for a review. I added the transition matrices (for both cases depending on the value remove_first_state) in the documentation. Please check if their definitions are aligned with what you had in mind. I updated the class body according to the definitions of these matrices.

@gdeleva gdeleva requested a review from jessegrabowski July 16, 2025 05:43
@gdeleva gdeleva changed the title Add num_steps_per_season parameter in TimeSeasonality Add duration parameter in TimeSeasonality Jul 17, 2025
Copy link
Contributor

@AlexAndorra AlexAndorra left a comment

Choose a reason for hiding this comment

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

Well done @gdeleva , this is looking good! Can you add tests to test_seasonality.py, inspiring you from the current tests (feel free to expand them actually, instead of just creating new ones, to avoid redundances). That way we make sure this is actually doing what we want it to do. Also test this when adding to other components, as shown in the corresponding issue

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.

You also need to update the logic for the initial_states. The user should only pick priors for s parameters, then you need to duplicate them with pt.repeat d times and save that results in self.ssm['initial_state']

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 really great! Just a few final nitpicks to reduce the number of tests, then let's merge it in. Thanks for this

Comment on lines 149 to 150
@pytest.mark.parametrize("d1", [1, 2, 3])
@pytest.mark.parametrize("d2", [1, 2, 3])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@pytest.mark.parametrize("d1", [1, 2, 3])
@pytest.mark.parametrize("d2", [1, 2, 3])
@pytest.mark.parametrize("d1, d2", [(1, 1), (1, 3), (3, 1), (3, 3)])

Our CI runtime is already pretty hefty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. Done.

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 No thank you for the support.

@gdeleva gdeleva requested a review from jessegrabowski July 21, 2025 12:08
@jessegrabowski jessegrabowski requested a review from Copilot July 21, 2025 12:25
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request adds a duration parameter to the TimeSeasonality class, which determines how long each seasonal period is held constant before moving to the next period. This enhancement allows for more flexible seasonal modeling where seasonal effects can persist for multiple time steps rather than changing at each step.

  • Adds a new duration parameter with default value of 1 to maintain backward compatibility
  • Updates the mathematical model documentation to explain both remove_first_state=True/False cases with the duration parameter
  • Modifies the internal state space representation to accommodate the duration parameter by repeating states and adjusting transition matrices

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
pymc_extras/statespace/models/structural/components/seasonality.py Implements the duration parameter in TimeSeasonality class with updated mathematical formulation, parameter validation, and symbolic graph construction
tests/statespace/models/structural/components/test_seasonality.py Comprehensive test updates to validate duration parameter functionality across all existing test scenarios
Comments suppressed due to low confidence (1)

pymc_extras/statespace/models/structural/components/seasonality.py:248

  • The default state naming pattern for duration > 1 uses the component name variable in the format f"{name}{i}{j}" but 'name' at this point contains the full component name like 'Seasonal[s=12, d=3]'. This will create confusing state names like 'Seasonal[s=12, d=3]_0_0'. Consider using a cleaner base name for state generation.
                state_names = [

@jessegrabowski
Copy link
Member

jessegrabowski commented Jul 21, 2025

Could you also rebase this onto the most up-to-date main? That's why the doc test is failing. There's been a bit of a nightmare with little bugs across several packages that is now resolved.

Copy link
Contributor

@AlexAndorra AlexAndorra left a comment

Choose a reason for hiding this comment

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

Looking great @gdeleva ! Plus, looks like it's already working with multiple observed time series, so well done 👏

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.

Great job!

@jessegrabowski jessegrabowski merged commit 718dae7 into pymc-devs:main Jul 21, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants