install ogcore package from pip

[jdebacker]

Resolves Issue #61

[jdebacker]

Test failures are with a DataFrame to_csv method call. Seems related to this issue with writing categorical variables to data files.

[rickecon]

@jdebacker. Two things.

1. You should probably remove .pre-commit-config.yaml from this PR. We shouldn't impose that workflow on contributors. I think we should recommend it as soon as we get a fix for the black version in that file.
2. You fixed the test_psid_data_setup.py failure, but now we have one test in test_income.py failing. This is strange because the only changes to income.py were two instances of spaces removed around exponentiation operators (via black). The error is in test_arctan_fit(), and it looks like it is just a small deviation in an assert np.allclose() function, likely from some numerical changes in either pandas or numpy.

   
   ogusa/tests/test_income.py ...F........                                  [ 94%]
   ...
       abil_deprec = 0.47
       test_vals = income.arctan_fit(
           first_point, coef1, coef2, coef3, abil_deprec, init_guesses
       )

  assert np.allclose(test_vals, expected_vals)

E assert False E + where False = <function allclose at 0x7fed9af82940>(array([30.19999164, 30.19998192, 30.19997105, 30.19995883, 30.19994498,\n 30.19992914, 30.19991088, 30.19988956, ...975104, 30.19969167, 30.19961251, 30.19950169,\n 30.19933545, 30.19905839, 30.19850427, 30.19684187, 14.19400013]), array([30.19999399, 30.19998699, 30.19997918, 30.19997039, 30.19996043,\n 30.19994904, 30.1999359 , 30.19992057, ...982094, 30.19977824, 30.19972131, 30.1996416 ,\n 30.19952204, 30.19932277, 30.19892423, 30.19772859, 14.19399974])) E + where <function allclose at 0x7fed9af82940> = np.allclose

ogusa/tests/test_income.py:99: AssertionError

[jdebacker]

@jdebacker. Two things.

You should probably remove .pre-commit-config.yaml from this PR. We shouldn't impose that workflow on contributors. I think we should recommend it as soon as we get a fix for the black version in that file.

I agree - done.

You fixed the test_psid_data_setup.py failure, but now we have one test in test_income.py failing. This is strange because the only changes to income.py were two instances of spaces removed around exponentiation operators (via black). The error is in test_arctan_fit(), and it looks like it is just a small deviation in an assert np.allclose() function, likely from some numerical changes in either pandas or numpy.

Yes. And this test passes for me locally with Pandas 1.4.1, Numpy 1.21.5, and Python 3.10.

[codecov-commenter]

Codecov Report

Merging #62 (088bc75) into master (b0db1ee) will decrease coverage by 0.84%. The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
- Coverage   79.19%   78.35%   -0.85%     
==========================================
  Files          19       19              
  Lines        1298     1303       +5     
==========================================
- Hits         1028     1021       -7     
- Misses        270      282      +12     

Flag	Coverage Δ
unittests	78.35% <87.50%> (-0.85%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
ogusa/tests/test_psid_data_setup.py	48.14% <0.00%> (ø)
ogusa/income.py	100.00% <100.00%> (ø)
ogusa/psid_data_setup.py	100.00% <100.00%> (ø)
ogusa/tests/test_income.py	87.62% <100.00%> (-12.38%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3ed4877...088bc75. Read the comment docs.

[rickecon]

@jdebacker. This is hard to test how to change the test_arctan_fit() function in test_income.py. This test passes locally on my machine as well. If we just update the expected_vals vector, this test will break again the next time numpy or pandas is updated. The numpy.allcose() command has the following form, as quoted from the linked documentation.

If the following equation is element-wise True, then allclose returns True. absolute(a - b) <= (atol + rtol * absolute(b))

I recommend we add the following parameter values to the np.allclose() command in test_arctan_fit(). Under brief inspection of the error traceback, it seems like an absolute error of atol=1e-4 will be sufficient, setting rtol=0.

assert np.allclose(test_vals, expected_vals, atol=1e-4, rtol=0.0)

I tested this on my machine. Obviously, if the test passed locally, it would pass again with this looser restriction. But I just wanted to make sure I had the syntax correct. I have submitted this change as a PR to your branch.

[jdebacker]

For the test_income.py failure, I'm marking that as local-only test. It passes on Python 3.10 and 3.9, but fails on <= 3.8 and the tolerance for it to pass there would have to be 1e-3, which is too loose. Rather just have a local test unless we could configure so that test only runs on Py >= 3.9.

[rickecon]

@jdebacker. Sounds good. I will merge this as soon as it passes all the tests. Then I will update PR #60, and that will be ready for your review. I am closing my PR to your branch.

[rickecon]

@jdebacker. Ah, there it is--the E ImportError: Numba needs NumPy 1.21 or less error. I did some digging on this and document it in PR #60 that numba does not currently support numpy>1.21. My recommendation in that PR is to temporarily pin the NumPy version in environment.yml to numpy<=1.21.2. I left a comment in the environment.yml file. You can merge that PR, then update this PR, and everything should pass.

[jdebacker]

Sounds good @rickecon. I merged your PR #60 into this branch.

[rickecon]

@jdebacker. This error in the build (3.10) tests is brutal. During the install of the ogusa-dev environment with Python 3.10, it can't find a set of required packages that are compatible. I re-ran the Python 3.10 tests four times and got the same error each time. I thought that it might just be a weird issue with downloading and installing miniconda, but it was not.

I think we either have to temporarily remove the Python 3.10 testing and temporarily pin the Python version to python<=3.9, or we have to do some pretty serious detective work as to what configuration of packages we need for this. I suggest we do the first option and forego Python 3.10 testing for OG-USA right now. I think this will resolve itself in a few months with updates to the package dependencies we are using.

See the PR I just made to @jdebacker's branch for this PR.

[rickecon]

@jdebacker. Thanks for all the work on this. LGTM.