Skip to content

Conversation

@mmcky
Copy link
Contributor

@mmcky mmcky commented Jul 1, 2024

This PR addresses #390

STATUS: IN-WORK

  • Check why the index starts from 1915 in 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).
  • Set global 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.html
  • variable name m_seq. Comment: removed as not used.
  • Remove dots and connect them. @HumphreyYang do you recall what this comment means. Should the figures be updated with connected line graphs?
  • Add a discussion on money supply and price level. Note: won't address in this PR.
  • Is "1/cents per polish mark" the same as "polish marks per cent"? In which case let's change to "Polish marks per US cent". (see @SylviaZhaooo or @longye-tian below)

@mmcky mmcky added the in-work label Jul 1, 2024
@netlify
Copy link

netlify bot commented Jul 1, 2024

Deploy Preview for taupe-gaufre-c4e660 ready!

Name Link
🔨 Latest commit 7dd069b
🔍 Latest deploy log https://app.netlify.com/sites/taupe-gaufre-c4e660/deploys/669f3aad10ab3200086f84d9
😎 Deploy Preview https://deploy-preview-494--taupe-gaufre-c4e660.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@HumphreyYang
Copy link
Member

HumphreyYang commented Jul 1, 2024

  • Remove dots and connect them. @HumphreyYang do you recall what this comment means. Should the figures be updated with connected line graphs?

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.

@github-actions
Copy link

github-actions bot commented Jul 1, 2024

@github-actions github-actions bot temporarily deployed to pull request July 1, 2024 05:23 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 1, 2024 05:24 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 22, 2024 01:12 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 22, 2024 01:14 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 22, 2024 01:38 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 22, 2024 01:39 Inactive
@mmcky
Copy link
Contributor Author

mmcky commented Jul 22, 2024

  • Remove dots and connect them. @HumphreyYang do you recall what this comment means. Should the figures be updated with connected line graphs?

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

@mmcky
Copy link
Contributor Author

mmcky commented Jul 23, 2024

@SylviaZhaooo or @longye-tian would you mind helping to finish off this PR.

Is "1/cents per polish mark" the same as "polish marks per cent"? In which case let's change to "Polish marks per US cent".

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.

@longye-tian
Copy link
Collaborator

@SylviaZhaooo or @longye-tian would you mind helping to finish off this PR.

Is "1/cents per polish mark" the same as "polish marks per cent"? In which case let's change to "Polish marks per US cent".

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,
Longye

@longye-tian
Copy link
Collaborator

@SylviaZhaooo or @longye-tian would you mind helping to finish off this PR.

Is "1/cents per polish mark" the same as "polish marks per cent"? In which case let's change to "Polish marks per US cent".

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 @mmcky ,

I just committed the changes for all the currencies on the label.

What do you think? Would you like to change something?

Best,
Longye

@github-actions github-actions bot temporarily deployed to pull request July 23, 2024 05:03 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 23, 2024 05:05 Inactive
@mmcky mmcky marked this pull request as ready for review July 23, 2024 05:07
@mmcky
Copy link
Contributor Author

mmcky commented Jul 23, 2024

thanks @longye-tian -- this looks ready to merge.

@mmcky mmcky added ready and removed in-work labels Jul 23, 2024
@github-actions github-actions bot temporarily deployed to pull request July 23, 2024 05:14 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 23, 2024 05:16 Inactive
@mmcky mmcky merged commit 0216cc1 into main Jul 23, 2024
@mmcky mmcky deleted the i390 branch July 23, 2024 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants