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

[WIP] Unit test firm.py #312

Merged
merged 11 commits into from
Aug 7, 2017
Merged

[WIP] Unit test firm.py #312

merged 11 commits into from
Aug 7, 2017

Conversation

hdoupe
Copy link
Contributor

@hdoupe hdoupe commented Jul 19, 2017

So far, I've written unit tests for test_get_r, test_get_w, test_get_Y, and test_get_L.

For test_get_r, test_get_w, and test_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 the TPI method is used. Are n and e a list of matrices of length T where each matrix is s x j?

I'm working on the remaining functions now. I welcome all feedback.

@jdebacker
Copy link
Member

@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 get_L() is called from the TPI solution method, it gets passed 3 dimensional arrays. n has dimensions [time, age, ability] so it might be as big as [T+S, S, J] (though for some parts of the time path, the first two dimensions are smaller). The matrix e has dimensions [age, ability] but is tiled in the time path so that it has the same dimensions as the other objects it interacts with in get_L and other functions.

@hdoupe
Copy link
Contributor Author

hdoupe commented Jul 19, 2017

@jdebacker said

Re the dimensions - yes, when get_L() is called from the TPI solution method, it gets passed 3 dimensional arrays. n has dimensions [time, age, ability] so it might be as big as [T+S, S, J] (though for some parts of the time path, the first two dimensions are smaller). The matrix e has dimensions [age, ability] but is tiled in the time path so that it has the same dimensions as the other objects it interacts with in get_L and other functions.

Ok that makes sense.

What dimensions are you expecting when get_L returns? Right now, I'm getting a scalar back from get_L when method="SS" and a vector of length T when method="TPI"

@jdebacker
Copy link
Member

@hdoupe says:

What dimensions are you expecting when get_L returns? Right now, I'm getting a scalar back from get_L when method="SS" and a vector of length T when method="TPI"

That's correct.

@PeterDSteinberg
Copy link
Contributor

Hi @hdoupe - I'm not familiar enough with the subject matter to comment on the math directly, but this looks like good py.test style of unit tests.

@hdoupe
Copy link
Contributor Author

hdoupe commented Jul 28, 2017

@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
Copy link
Member

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

r = np.array([1.0, 1.0])
gamma = 0.5
tau_b = 0.75
delta = 4.0
Copy link
Member

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.

gamma = 0.5
tau_b = 0.75
delta = 4.0
delta_tau = 4.0/3.0
Copy link
Member

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
Copy link
Member

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.

@hdoupe
Copy link
Contributor Author

hdoupe commented Jul 31, 2017

@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.

@jdebacker
Copy link
Member

@hdoupe How is this PR looking? Ready for merge?

@hdoupe
Copy link
Contributor Author

hdoupe commented Aug 7, 2017

@jdebacker I think it's ready to merge. I used the parameters that you suggested. Are you satisfied with the changes?

@jdebacker
Copy link
Member

Yes. Thanks @hdoupe. Merging now.

@jdebacker jdebacker merged commit 6e251fc into PSLmodels:master Aug 7, 2017
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.

4 participants