- 
                Notifications
    You must be signed in to change notification settings 
- Fork 72
          Remove k_endog & k_exog parameters in SSM
          #599
        
          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
base: main
Are you sure you want to change the base?
Conversation
| Hey @jessegrabowski, after updating all the models and the regression component in the structural api the complexity of the validation code dropped quite a bit. I am looking at pulling out that VARMAX validation utility to use across all the other models (SARIMAX, ETS, DFM) but the only commonality between them is how the  | 
| I was going to say yes anyway :D | 
| Jesse, I marked the two open issues for closer upon merging this PR. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work as always! Left some feedback.
Also rebase, we just merged the Pytensor update that will fix tests.
| def __init__( | ||
| self, | ||
| k_exog: int | None = None, | ||
| # k_exog: int | None = None, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete, no need to leave flotsam in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My apologies, I missed that. I will delete it in the next push.
| return exog_dims | ||
|  | ||
|  | ||
| def _validate_endog_names(endog_names) -> int: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this to validate_names, and add a optional: bool =True argument so it can either error or return None.  Then you can eliminate _get_state_names in the regression component and use this instead. Also can use it for the exog names in SARIMAX, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a var_name argument as well so that if it does raise a value error the message will clearly state what variable is missing.
|  | ||
| @pytest.mark.filterwarnings( | ||
| "ignore::RuntimeWarning" | ||
| ) # Needed this due to RuntimeWarning: divide by zero encountered in matmul | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you on a Mac? This is a known bug with Mx chips, see here: numpy/numpy#29820
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I am on a Mac. Should I run in x86 using rosetta instead until they fix the issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No its fine, it doesn't actually do anything. But you don't need to filterwarnings either. You can just do it locally for your own testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that is reasonable. I removed the filter before pushing new commits.
| # def test_create_varmax_with_exogenous_k_exog_int(self, data): | ||
| # mod = BayesianVARMAX( | ||
| # endog_names=["realgdp", "realcons", "realinv"], | ||
| # order=(1, 0), | ||
| # exog_state_names=["exogenous_0", "exogenous_1"], | ||
| # verbose=False, | ||
| # measurement_error=False, | ||
| # stationary_initialization=False, | ||
| # ) | ||
| # assert mod.k_exog == 2 | ||
| # assert mod.exog_state_names == ["exogenous_0", "exogenous_1"] | ||
| # assert mod.data_names == ["exogenous_data"] | ||
| # assert mod.param_dims["beta_exog"] == ("observed_state", "exogenous") | ||
| # assert mod.coords["exogenous"] == ["exogenous_0", "exogenous_1"] | ||
| # assert mod.param_info["beta_exog"]["shape"] == (mod.k_endog, 2) | ||
| # assert mod.param_info["beta_exog"]["dims"] == ("observed_state", "exogenous") | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(also applies to other commented out blocks below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, will do! Didn't want to delete anything before you approve of it.
| } | ||
| else: | ||
| exog_names = exog_state_names or [f"exogenous_{i}" for i in range(k_exog)] | ||
| exog_names = exog_state_names | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be misunderstanding your question, but I removed the or statement because there no longer is a k_exog parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant why keep this, since all it does is rename a variable. There should be a more elegant way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are absolutely right. That is silly. I changed it to directly pass in exog_state_names.
… and updated tests accordingly
…exogenous variables and updated tests accordingly
…dog_names required and exog_state_names required for exogenous variables and updated tests accordingly
…quired and exog_names required if exogenous variables are requested and updated tests accordingly
…k_endog and k_exog parameters
…eused in both endog and some exog cases
9a3e24d    to
    330dc19      
    Compare
  
    
This PR is related to #589 and aims to remove redundant parameters
k_endogandk_exogfrom SSM models. The following models will be updated:In addition, this PR will also address
Closes #587
Closes #589