-
-
Notifications
You must be signed in to change notification settings - Fork 267
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
StateSpace Module Hurricane Case Study ~~WIP~~ Ready for Review #763
base: main
Are you sure you want to change the base?
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
View / edit / reply to this conversation on ReviewNB fonnesbeck commented on 2025-01-05T01:33:47Z Not sure if its my setup, but there are some stray $ characters in the intro. Dekermanjian commented on 2025-01-05T11:37:14Z Hi @fonnesneck! Thank you for taking the time to review this notebook. On ReviewNB it seems that the double $ that should generate an equation on its own line doesn't render, but it does on the PyMC examples page. That is why there are stray $ characters. |
View / edit / reply to this conversation on ReviewNB fonnesbeck commented on 2025-01-05T01:33:48Z We don't currently use plotly in any of the other examples, so need to update the requirements. Dekermanjian commented on 2025-01-05T11:41:24Z Yes, you are correct. I will move the comment on line 6 to the very top of this cell. I have Plotly included in the extra_installs in the myst.md file. Again this does not render in ReviewNB but it does on the PyMC examples website. You can see it if you visit the documentation preview https://pymcio--763.org.readthedocs.build/projects/examples/en/763/case_studies/ssm_hurricane_tracking.html |
View / edit / reply to this conversation on ReviewNB fonnesbeck commented on 2025-01-05T01:33:49Z spelling: "originate"
Also, maybe worth adding a sentence describing what defines an origination point. Dekermanjian commented on 2025-01-05T11:43:24Z Thank you for catching that! Yes, I agree with you. I will add some more context on what constitutes an origination point. |
Hi @fonnesneck! Thank you for taking the time to review this notebook. On ReviewNB it seems that the double $ that should generate an equation on its own line doesn't render, but it does on the PyMC examples page. That is why there are stray $ characters. View entire conversation on ReviewNB |
Yes, you are correct. I will move the comment on line 6 to the very top of this cell. I have Plotly included in the extra_installs in the myst.md file. Again this does not render in ReviewNB but it does on the PyMC examples website. You can see it if you visit the documentation preview https://pymcio--763.org.readthedocs.build/projects/examples/en/763/case_studies/ssm_hurricane_tracking.html View entire conversation on ReviewNB |
Thank you for catching that! Yes, I agree with you. I will add some more context on what constitutes an origination point. View entire conversation on ReviewNB |
2. Improved 24-hour ahead forecasts of simplest model 3. Added a section and model with how to add exogenous variables to the model
2. cleaned up typos and figure sizes/zoom
2. Added a non-linear B-Splines section
2. Added closing remarks section 3. Updated references bib file
View / edit / reply to this conversation on ReviewNB jessegrabowski commented on 2025-04-03T14:05:25Z Some nitpicks on the latex:
Dekermanjian commented on 2025-04-03T23:34:44Z Totally agree with all of the above. I will clean up the equations with latex and make a note on the Joseph form of the filtered covariance equation. I will also look up what the convention is for the t subscript and replace it everywhere in the notebook that occurs. Dekermanjian commented on 2025-04-06T21:46:55Z Hey Jesse, I think maybe I misunderstood one of your comments. The letter "t" itself isn't the issue in your comment but the fact that I have it subscript t within the text under the definition column of the above tables. Is that correct? |
View / edit / reply to this conversation on ReviewNB jessegrabowski commented on 2025-04-03T14:05:26Z Line #3. Generates a 95% CI ellipse via a chi-square multivariate normal approximation Nit: we use numpy docstring formatting in the pymc codebase, might as well use it here too Dekermanjian commented on 2025-04-03T23:35:39Z Thank you for making me aware of the numpy formatting. I will make the change everywhere this applies. |
View / edit / reply to this conversation on ReviewNB jessegrabowski commented on 2025-04-03T14:05:27Z Line #20. ): # First handful of ellipses are huge because of little data in the iterative nature of the Kalman filter
I'm reviewing this sequentially so haven't gotten to the model yet, but this might also be because the initial P0 matrix is too diffuse. Dekermanjian commented on 2025-04-03T23:37:51Z I didn't think of that. I will make a note of it in the text and I will play around with different P0 initializations. |
View / edit / reply to this conversation on ReviewNB jessegrabowski commented on 2025-04-03T14:05:27Z Line #21. # Hack that should be fixed in StateSpace soon Was this ever fixed? I should do it immediately if not. Dekermanjian commented on 2025-04-03T23:39:29Z I will test it out with the latest versions of pymc + pymc-extras and report back if it isn't. I recall this being fixed, though. Dekermanjian commented on 2025-04-06T14:14:22Z Hey Jesse, the GitHub issue for this issue is still open pymc-devs/pymc-extras#424 and I can confirm that the hack is still required on the latest version of pymc-extras. |
View / edit / reply to this conversation on ReviewNB jessegrabowski commented on 2025-04-03T14:05:28Z I guess the points are colored by category? Is it possible to add a legend for that? (This plot is super slick btw) Dekermanjian commented on 2025-04-03T23:41:24Z Yes, that is correct. I agree that a legend should be included. I will add one! |
View / edit / reply to this conversation on ReviewNB jessegrabowski commented on 2025-04-03T14:05:29Z In this example we assume constant acceleration (In order to keep our system of equations linear) I don't think constant acceleration is required for linearity in this setup -- you could have a stochastic innovation on the acceleration term that allows it to change over time, and indeed you do! I guess this is a technical point required to discretize the Newtonian ODEs, but it's also not technically true in your model. Make of that what you will.
Our states are the following components derived from the Newtonian equations of motion Not required, but you could show how these matrices arise from integrating the ODEs p_dot(t) = v(t), v_dot(t) = a(t), a_dot(t) = 0 over an interval delta t.
In this example we fix our observation/measurement error to a small value (0.1) reflecting our confidence in the measurements. The latex matrix H has 0.5 on the diagonal, not 0.1. Maybe also make a note about the units of the data, and what the number 0.1 actually represents in terms of measurement error.
variane (diagonals) covariance (off-diagonals) of the Newtonian equations Typo: variance
Dekermanjian commented on 2025-04-04T00:04:13Z Yes, you are correct. I think I need to clarify that with this model we aren't able to model any angular acceleration whenever the hurricane makes maneuvers so we aim to capture turns with the stochastic innovation on the acceleration term.
I will put in the ODE integrations. I think this will be good for completeness. Of course, I will fix the mismatch between the latex and the text and the typo. |
View / edit / reply to this conversation on ReviewNB jessegrabowski commented on 2025-04-03T14:05:30Z Line #13. simple_idata = pm.sample(nuts_sampler="numpyro", target_accept=0.95) Try with nutpie, you might be able to get away with not increasing target accept. Dekermanjian commented on 2025-04-04T00:05:03Z Yes, I will give it a go. I believe at the time the Jax backends were not implemented yet. I will give that a try too. |
View / edit / reply to this conversation on ReviewNB jessegrabowski commented on 2025-04-03T14:05:31Z It uses data from the entire time series in the sense that your estimated parameters already saw the entire future.
But all it does is re-write your model so that Dekermanjian commented on 2025-04-04T00:06:04Z Thank you so much for clarifying this for me! jessegrabowski commented on 2025-04-04T03:46:16Z It happens here if you're interested:
|
View / edit / reply to this conversation on ReviewNB jessegrabowski commented on 2025-04-03T14:05:32Z The addition to the R matrix imply that the exogenous parameters do not exhibit any process noise. Nit: I wouldn't describe the stochastic innovations in the transition equation as "noise", since they're in some sense "real", as updates to the values of the latent states of the system. I think of "noise" as arising from some flaw in our ability to observe, e.g. from the measurement errors. Dekermanjian commented on 2025-04-04T00:10:53Z I will go back and reword how the stochastic innovations are described in the text. |
View / edit / reply to this conversation on ReviewNB jessegrabowski commented on 2025-04-03T14:05:32Z Maybe a forest plot here? It's a bit more compact.
You might also have to play with the prior variances on these. N(0, 10) is pretty aggressive, and you can see that it's not like an OLS regression where the priors quickly wash out. All these posteriors have pretty wide uncertainties. I'm not familiar enough with the science to know if an effect size of -20 * wind pressure is realistic, but your priors but non-zero probability mass on that.
Dekermanjian commented on 2025-04-04T00:13:43Z I agree, that priors are poorly specifies. I will go back and pick more sensible priors and I will condense the output with a forest plot. |
View / edit / reply to this conversation on ReviewNB jessegrabowski commented on 2025-04-03T14:05:33Z To keep things simple, we are going to define a constant number of knots that are equal in both variables (same #knots for both longitude and latitude) and we are going to place the knots using a quantile function over the variable space. You can see the knots plotted out below for each variable. Write "same number of knots" -- currently looks like a hashtag
Dekermanjian commented on 2025-04-04T00:14:56Z Ah yes, I should've been more careful. I will make the change. |
View / edit / reply to this conversation on ReviewNB jessegrabowski commented on 2025-04-03T14:05:34Z Line #201. "exog_dims": [ Could use an iterator + f-string for these names; they're a bit verbose at the moment Dekermanjian commented on 2025-04-04T00:16:14Z Yes, totally agree. I will make the change. |
View / edit / reply to this conversation on ReviewNB jessegrabowski commented on 2025-04-03T14:05:35Z Line #6. P0 = pt.eye(40) * 1
I guess this used to be multiplied by something else? Dekermanjian commented on 2025-04-04T00:19:32Z Yeah, I must've been testing different variances for P0. I will get rid of the redundant '*1'. |
View / edit / reply to this conversation on ReviewNB jessegrabowski commented on 2025-04-03T14:05:36Z This needs a bit of a spit polish. The last sentence is not a complete sentence. The logic is also a bit hard to follow: Newtonian model is the best, but adding exogenous variables is better?
I think one issue is that in the exogenous 24-hour forecasts, you're passing in the real exogenous data from the future, so have some look-ahead bias there. A more "fair" test would be to extend the last observed value out for 4 periods, or do some kind of simple trend. This is a relatively small point though, because the purpose of the notebook isn't the forecasting exercise per se, so just take it under advisement. Dekermanjian commented on 2025-04-04T00:27:24Z Yes, you are absolutely correct. I shouldn't be passing in the real exogenous data from the future. I will extend the last observed value and look into if I can also use lagged variables. I also agree that this section needs to read better and be clearer about what are the conclusions. |
This is really amazing work! Sorry I slept on this for so long. Structurally I think it's good to go, it just need a bit of polish in a few sections. The only major revision I would consider is not using out-of-sample exogenous data to do the exogenous forecasts, to make it more fair against the kinematics-only model. You could also consider adding a more formal "test" of each model, for example the RMSE of each point the predicted path vs the observed path as a time series. You can compute that sample-wise and plot the mean and HDI for each model against each-other, showing the change in expected error, and the variance in the errors, over time. Also quite curious why your model is so hostile to Newfoundland :) |
Totally agree with all of the above. I will clean up the equations with latex and make a note on the Joseph form of the filtered covariance equation. I will also look up what the convention is for the t subscript and replace it everywhere in the notebook that occurs. View entire conversation on ReviewNB |
Thank you for making me aware of the numpy formatting. I will make the change everywhere this applies. View entire conversation on ReviewNB |
I didn't think of that. I will make a note of it in the text and I will play around with different P0 initializations. View entire conversation on ReviewNB |
I will test it out with the latest versions of pymc + pymc-extras and report back if it isn't. I recall this being fixed, though. View entire conversation on ReviewNB |
Yes, that is correct. I agree that a legend should be included. I will add one! View entire conversation on ReviewNB |
Yes, you are correct. I think I need to clarify that with this model we aren't able to model any angular acceleration whenever the hurricane makes maneuvers so we aim to capture turns with the stochastic innovation on the acceleration term.
I will put in the ODE integrations. I think this will be good for completeness. Of course, I will fix the mismatch between the latex and the text and the typo. View entire conversation on ReviewNB |
Yes, I will give it a go. I believe at the time the Jax backends were not implemented yet. I will give that a try too. View entire conversation on ReviewNB |
Thank you so much for clarifying this for me! View entire conversation on ReviewNB |
I will go back and reword how the stochastic innovations are described in the text. View entire conversation on ReviewNB |
I agree, that priors are poorly specifies. I will go back and pick more sensible priors and I will condense the output with a forest plot. View entire conversation on ReviewNB |
Ah yes, I should've been more careful. I will make the change. View entire conversation on ReviewNB |
Yes, totally agree. I will make the change. View entire conversation on ReviewNB |
Yeah, I must've been testing different variances for P0. I will get rid of the redundant '*1'. View entire conversation on ReviewNB |
Yes, you are absolutely correct. I shouldn't be passing in the real exogenous data from the future. I will extend the last observed value and look into if I can also use lagged variables. I also agree that this section needs to read better and be clearer about what are the conclusions. View entire conversation on ReviewNB |
Thank you so much for taking time to review this work!! I will include your suggestion of a more formal test for each model to fairly compare performance.
|
It happens here if you're interested:
View entire conversation on ReviewNB |
Hey Jesse, the GitHub issue for this issue is still open pymc-devs/pymc-extras#424 View entire conversation on ReviewNB |
Ok, I'll work on that ASAP |
Hey Jesse, I think maybe I misunderstood one of your comments. The letter "t" itself isn't the issue in your comment but the fact that I have it subscript t within the text under the definition column of the above tables. Is that correct? View entire conversation on ReviewNB |
2. Restructured function docstrings to comply with numpy's guidestyle 3. Added ODE derivation of Newtonian equations of motion 4. Added legend to origins scatterplot 5. Fixed issue with passing in future covariates into exogenous model when forecasting 6. Added model evaluations and comparisons 7. Fixed issue with diffuse initialization of P0
Hey @jessegrabowski, I believe I made all of the changes that you suggested. There is one thing that I can't figure out how to do, though. I want to cite your notebook in pymc-extras but I can't figure out how to do so following the pymc-examples guide. Do you happen to know how to do this? |
Case Study to showcase the use of the State Space Module in
pymc-extras
This relates to #715
In this case study I have done the following:
I still wish to add the following:
@jessegrabowski Once I have completed this, would you be willing to be a reviewer for this case study?
EDIT:
I am thinking about skipping the VARMAX model section as this is starting to get a bit long, in my opinion.
I am open to feedback from others on whether to skip or add this section.
EDIT 2:
This is now ready for review.
EDIT 3:
I need some help with referencing a notebook in a repo. Specifically, this
I've read through the Jupyter Style documentation but it is still not apparent to me how to add a reference to a notebook in a repo that doesn't have a published sphinx documentation page.
📚 Documentation preview 📚: https://pymc-examples--763.org.readthedocs.build/en/763/