-
-
Notifications
You must be signed in to change notification settings - Fork 122
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
[WIP] Unit test firm.py #312
Conversation
@hdoupe I think the looping is a good idea since there may be array broadcasting errors in the vectorized code. Re the dimensions - yes, when |
@jdebacker said
Ok that makes sense. What dimensions are you expecting when |
@hdoupe says:
That's correct. |
Hi @hdoupe - I'm not familiar enough with the subject matter to comment on the math directly, but this looks like good |
@PeterDSteinberg Thanks for the review. @jdebacker @rickecon What do you think of these tests? Are you OK with the values that I chose to test these functions? Are there any other tests that you want me to add for the firms module? |
ogusa/tests/test_firm.py
Outdated
gamma = 0.5 | ||
delta = 0.25 | ||
tau_b = 0.5 | ||
delta_tau = 1.0 |
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.
@hdoupe Let's make delta_tau something just a bit greater than delta. So if delta =0.25, let's make delta_tau 0.35
ogusa/tests/test_firm.py
Outdated
r = np.array([1.0, 1.0]) | ||
gamma = 0.5 | ||
tau_b = 0.75 | ||
delta = 4.0 |
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.
delta needs to be in [0, 1]. Try 0.15.
ogusa/tests/test_firm.py
Outdated
gamma = 0.5 | ||
tau_b = 0.75 | ||
delta = 4.0 | ||
delta_tau = 4.0/3.0 |
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.
delta_tau also needs to be in [0, 1]. Try delta * 0.2.
|
||
epsilon = 0.5 | ||
Z = 4.0 | ||
tau_b = 0.0 |
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.
Testing with tau_b > 0 above, so this is another good check. But wouldn't want to only test with tau_b = 0.
@PeterDSteinberg do you know what's causing the build to fail? It looks like it can't find taxcalc package version 0.8.3? [EDIT] Nevermind, it looks like the build was successful on the second commit. |
@hdoupe How is this PR looking? Ready for merge? |
@jdebacker I think it's ready to merge. I used the parameters that you suggested. Are you satisfied with the changes? |
Yes. Thanks @hdoupe. Merging now. |
So far, I've written unit tests for
test_get_r
,test_get_w
,test_get_Y
, andtest_get_L
.For
test_get_r
,test_get_w
, andtest_get_Y
, I chose values for the parameters that seemed realistic and made the calculations easier. I then checked to make sure the expected and actual values were equal.For
test_get_L
, I looped over the values of the matrices to do the multiplication. I wasn't sure about the behavior of Python's element wise matrix multiplication. So, I used the looping method because it is much easier to understand. That way we can make sure the matrix multiplication method is doing what we expect it to do.Also, I'm confused about the dimensions of the matrices that
test_get_L
expects when theTPI
method is used. Aren
ande
a list of matrices of lengthT
where each matrix iss
xj
?I'm working on the remaining functions now. I welcome all feedback.