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? |
| gamma = 0.5 | ||
| delta = 0.25 | ||
| tau_b = 0.5 | ||
| delta_tau = 1.0 |
There was a problem hiding this comment.
@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
| r = np.array([1.0, 1.0]) | ||
| gamma = 0.5 | ||
| tau_b = 0.75 | ||
| delta = 4.0 |
There was a problem hiding this comment.
delta needs to be in [0, 1]. Try 0.15.
| gamma = 0.5 | ||
| tau_b = 0.75 | ||
| delta = 4.0 | ||
| delta_tau = 4.0/3.0 |
There was a problem hiding this comment.
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.
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_Lexpects when theTPImethod is used. Arenandea list of matrices of lengthTwhere each matrix issxj?I'm working on the remaining functions now. I welcome all feedback.