-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
[equalizing_difference] Update numpy
code in to remove vectorization
#388
Conversation
✅ Deploy Preview for taupe-gaufre-c4e660 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Wow -- nice work. this reduces a LOT of code. |
Many thanks @mmcky, Please let me know if you approve this PR. Once this is merged, I will work on improving the |
lectures/equalizing_difference.md
Outdated
# one switches to the weak model by setting π | ||
self.R, self.γ_h, self.γ_c, self.w_h0, self.D = R, γ_h, γ_c, w_h0, D | ||
self.T, self.π = T, π | ||
# Tweaked model |
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.
After π=None
put # Extra parameter (modified version)
and then replace Tweaked model
with Modification for reinterpreted model
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.
Hi @jstac,
Since namedtuple
for the entrepreneur model when moving it into the exercise?
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.
Hi @HumphreyYang , yes, good call. Let's do exactly that.
lectures/equalizing_difference.md
Outdated
|
||
```{code-cell} ipython3 | ||
R_arr = np.linspace(1, 1.2, 50) | ||
plt.plot(R_arr, φ_R(ex1, R_arr)) | ||
plt.plot(R_arr, compute_gap(create_edm(R=R_arr))) |
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.
Wow, this actually works?? That's amazing. So create_edm(R=R_arr)
returns a numpy array of namedtuples??
This definitely needs some comments.
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 think this is passing the model with parameter R
as a numpy
array into compute_gap
but not creating a numpy
array of namedtuple
.
The R_arr
is then unpacked in the compute_gap
function, and the operation on the array is broadcasted within the function.
This definitely needs some comments.
I agree! Would you think a comment like # Create a model and compute the gap
suffice?
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.
Please see my response below.
lectures/equalizing_difference.md
Outdated
We find that raising the gross interest rate $R$ increases the initial college wage premium $\phi$, as we did with our graphical analysis earlier |
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.
"as we did with our graphical analysis earlier" -> "in line with our earlier graphical analysis"
Thanks @HumphreyYang , please see my comments. @longye-tian , do you have any suggestions or are you happy with these changes? |
@jstac Thank you for asking me. I learn a lot from how @HumphreyYang improve the code. It's really amazing how he creates the array of namedtuples. Maybe we can create some variables to store this array of namedtuples and the gap values using list comprehension.
Best, |
Thanks @longye-tian for the nice suggestion. @HumphreyYang , your code is elegant, fast and concise but @longye-tian 's version is more explicit and I think it's better for this intro lecture series. Can you please switch to his version? |
Many thanks @jstac and @longye-tian for the suggestions. I will switch to the more explicit version. |
Very nice work @HumphreyYang and many thanks @longye-tian for helpful suggestions. This is a big improvement. Merging. |
This PR completes the first stage of #385.