-
Notifications
You must be signed in to change notification settings - Fork 3
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
added data perprocessing and linear model file #33
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 |
@@ -0,0 +1,717 @@ | |||
{ |
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.
Can you add some headers to the sections in this notebook like you have in the other one (Dowload data, Detrend data, etc)
Reply via ReviewNB
@@ -0,0 +1,717 @@ | |||
{ |
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.
Instead of dropping the trends later, I think it would be better to make df_trend = df.copy()
here, then save and plot them separately:
for col in log_cols: cyc, trend = hpfilter(np.log(df[col].dropna()), lamb=1600) df.loc[df[col].dropna().index, col] = cyc df_trend.loc[df[col].dropna().index, col] = trend
Reply via ReviewNB
@@ -0,0 +1,717 @@ | |||
{ |
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.
There's a helper function in gEconpy.plotting
called prepare_gridspec_figure
,it sets up arrays of subplots for non-square numbers of plots, and will prevent you from having these two empty ones at the end. You can see how it is used here: https://github.com/jessegrabowski/gEconpy/blob/cd2ae75747b0aa8bc23a6b35033f3a3e4ca7bc79/gEconpy/plotting/plotting.py#L237
Reply via ReviewNB
@@ -0,0 +1,717 @@ | |||
{ |
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 like the heatmaps you use in the other notebook better than just a table, so the viewer's eye can follow the colormap. Personally, blocks of numbers just wash over me like I'm on ketamine.
Reply via ReviewNB
@@ -0,0 +1,376 @@ | |||
{ |
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.
@@ -0,0 +1,376 @@ | |||
{ |
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.
Line #2. print('#' * 20, 'SOLVE MODEL', '#'*20)
For print like this you can do print(' SOLVE MODEL '.center(50, '#'))
(the number is the total line length, including the SOLVE MODEL part
Reply via ReviewNB
@@ -0,0 +1,376 @@ | |||
{ |
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.
Can you pass shock sizes to silence those warnings? Even if you just use the default. Since they're all the same you can just pass a covariance matrix:
mod1.compute_stationary_covariance_matrix(shock_cov_matrix=np.eye(3) * 0.01)
Reply via ReviewNB
@@ -0,0 +1,376 @@ | |||
{ |
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.
Line #3. mask[np.triu_indices_from(mask)] = True
Maybe use k=1 in triu_indices_from so that you get the main diagonal in the plots, as in mask[np.triu_indices_from(mask, k=1)] = True
? The variance is as interesting as the covariances.
Reply via ReviewNB
@@ -0,0 +1,376 @@ | |||
{ |
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.
Line #1. from src.utils import plot_dfs
I think it looks nicer to keep all the imports at the top of the notebook
Reply via ReviewNB
@@ -0,0 +1,376 @@ | |||
{ |
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.
Line #7. plot_dfs(dfs, sns.lineplot, legend=[i.base_name for i in mod.shocks]);
There's a plotting function for IRFs in gEconpy.plotting.plot_irf
,was it not suitable for your needs? I should tune it up if so, so you don't need to write any custom plotting functions in a notebook like this.
Reply via ReviewNB
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.
This is really great! I made a couple small comments. I think it would be good if you could roll all 3 files (the two notebooks and the utils) into a single file. My dream is to set up an "example gallery" like PyMC/Arviz has, so these two notebooks combined would be a great addition to that.
If you have extra time, my "stretch goal" would be a bit of commentary/analysis of the results. That would give it a bit more pedagogical value for someone who wants to do an analysis like this. Let me know if you want some help writing that up.
examples/src/utils.py
Outdated
from itertools import chain | ||
|
||
|
||
def plot_dfs(dfs, plot_func, dfs_cov: pd.DataFrame=None, when_crisis: pd.Series=None, legend: list=None, no_cols: int=3, figsize: tuple=(5, 2), **kwargs): |
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.
Is it possible to use the "built-in" plotting functions? It'd be nice to avoid a utils file for an example notebook, and as I remarked in the notebook, if those functions don't do what you need them to do, I need to fix them up
########################################################################### | ||
# New Keynesian model (Galì, 2008) | ||
########################################################################### | ||
|
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 an options flag you can use in linear models that will automatically pass linear_model = True
to places like steady_state
and solve_model
. You can see an example here: https://github.com/jessegrabowski/gEconpy/blob/main/tests/Test%20GCNs/RBC_Linearized.gcn
Also you can ignore the CI failure, it has nothing to do with your PR |
I have added some data preprocessing and the Galì 2008 canonical DSGE in linear as well as FOC form.