Skip to content
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

long_run_growth: styling and label fixes #328

Merged
merged 2 commits into from
Dec 20, 2023
Merged

long_run_growth: styling and label fixes #328

merged 2 commits into from
Dec 20, 2023

Conversation

kp992
Copy link
Contributor

@kp992 kp992 commented Dec 17, 2023

Fixes #291

Copy link

netlify bot commented Dec 17, 2023

Deploy Preview for taupe-gaufre-c4e660 ready!

Name Link
🔨 Latest commit 267f368
🔍 Latest deploy log https://app.netlify.com/sites/taupe-gaufre-c4e660/deploys/65824738fc52b0000861a0e0
😎 Deploy Preview https://deploy-preview-328--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.

Copy link

github-actions bot commented Dec 17, 2023

@mmcky mmcky requested a review from HumphreyYang December 18, 2023 05:29
@mmcky
Copy link
Contributor

mmcky commented Dec 18, 2023

thanks @kp992 I will ask @HumphreyYang to review as he recently edited this lecture.

Copy link
Collaborator

@HumphreyYang HumphreyYang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks @kp992! It looks great. Just two very small comments. Please let me know your thoughts.

thanks @kp992 I will ask @HumphreyYang to review as he recently edited this lecture.

(I think I edited the inflation lecture instead of long_run_growth) : )

@@ -518,19 +504,17 @@ BEM = ['GBR', 'IND', 'AUS', 'NZL', 'CAN', 'ZAF']
gdp['BEM'] = gdp[BEM].loc[start_year-1:end_year].interpolate(method='index').sum(axis=1) # Interpolate incomplete time-series
Copy link
Collaborator

@HumphreyYang HumphreyYang Dec 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line is also a bit too long. I suggest we can cut it to this:

Suggested change
gdp['BEM'] = gdp[BEM].loc[start_year-1:end_year].interpolate(method='index').sum(axis=1) # Interpolate incomplete time-series
# Interpolate incomplete time-series
gdp['BEM'] = gdp[BEM].loc[start_year-1:end_year].interpolate(method='index').sum(axis=1)


Let's take a look at the aggregation that represents the British Empire.

```{code-cell} ipython3
gdp['BEM'].plot() # The first year is np.nan due to interpolation
gdp['BEM'].plot(ylabel="International $'s") # The first year is np.nan due to interpolation
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line prints out the label
Screenshot

We can suppress printing using

Suggested change
gdp['BEM'].plot(ylabel="International $'s") # The first year is np.nan due to interpolation
_ = gdp['BEM'].plot(ylabel="International $'s") # The first year is np.nan due to interpolation

or

Suggested change
gdp['BEM'].plot(ylabel="International $'s") # The first year is np.nan due to interpolation
gdp['BEM'].plot(ylabel="International $'s") # The first year is np.nan due to interpolation
plt.show()

Copy link
Contributor

@mmcky mmcky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @kp992 and @HumphreyYang I think these are good changes.

@mmcky
Copy link
Contributor

mmcky commented Dec 20, 2023

thanks @kp992 and @HumphreyYang.

This PR just adjusts the spacing of the code to improve readability. I don't see any changes to labels -- is that right?

@HumphreyYang
Copy link
Collaborator

This PR just adjusts the spacing of the code to improve readability. I don't see any changes to labels -- is that right?

I think there is only one change in the label at L510.

@mmcky
Copy link
Contributor

mmcky commented Dec 20, 2023

This PR just adjusts the spacing of the code to improve readability. I don't see any changes to labels -- is that right?

I think there is only one change in the label at L510.

Thanks @HumphreyYang nice find. 👍

@mmcky
Copy link
Contributor

mmcky commented Dec 20, 2023

@thomassargent30 I am merging this as mainly improving the formatting in the md file. There is one visual change adding one label to a graph on L510

@mmcky mmcky merged commit 8923f9e into main Dec 20, 2023
4 checks passed
@mmcky mmcky deleted the i291 branch December 20, 2023 23:37
@kp992
Copy link
Contributor Author

kp992 commented Dec 21, 2023

Thanks @mmcky and @HumphreyYang

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Labels for figures in long_run_growth
3 participants