-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
[inflation_history] ENH: Implement review suggestions and comments #494
Conversation
✅ Deploy Preview for taupe-gaufre-c4e660 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Many thanks @mmcky. I think this is referring to the moving average plot, and we were discussing whether should we remove the data points and leave the moving average line or just connect the monthly inflation data points. Some think the current version looks good but some suggest removing the points in the graph will look cleaner. I think this is why it is not implemented. Please let us know your thoughts. |
thanks @HumphreyYang I think using the moving average line only is best here. I have adjusted in 58d65ee |
@SylviaZhaooo or @longye-tian would you mind helping to finish off this PR.
I can confirm we need to change these cases as suggested above. This applies to Polish marks (+ other currencies as well). It will require a bit of rework of the code (i.e. pandas columns) in addition to the graph titles that are generated. |
Hi Matt, Sure thing. I will look into it and let you know if I'm stuck. Best, |
Hi Matt @mmcky , I just committed the changes for all the currencies on the label. What do you think? Would you like to change something? Best, |
thanks @longye-tian -- this looks ready to merge. |
This PR addresses #390
STATUS: IN-WORK
df_fig5_bef1914
. Comment: updated variable name to align with text that ends at 1914. The index ends at 1914 in the dataframe (and used to end at 1915 due to filter value in pandas).dpi
default. Comment: This was an indirect way to make the specific figure in question larger (due to the content). This is a special case figure and so I have updated it to specify a figsize to ensure it is large enough to show the content. This is more in line with out policy https://manual.quantecon.org/styleguide/figures.htmlm_seq
. Comment: removed as not used.