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

Refactor dropq functions #1314

Merged
merged 8 commits into from
May 2, 2017
Merged

Refactor dropq functions #1314

merged 8 commits into from
May 2, 2017

Conversation

martinholmer
Copy link
Collaborator

This pull request implements the proposal discussed in deploy repo issue 45.

The refactoring of the dropq code involves moving all functions that are not "exported" in the taxcalc/dropq/__init__.py file from dropq.py to drop_utils.py. Another main objective of the refactoring is to eliminate the code duplication in the two run_nth_year_*_model functions. And finally, a number of obsolete functions and tests were removed and some new tests were added.

There has been just one change in the public dropq API. The "exported" function formerly named run_nth_year_model has been renamed run_nth_year_tax_calc_model. This makes it clear which model is being run and make the function name analogous to the run_nth_year_gdp_elast_model function.

While these changes should be tested in TaxBrain, it would appear that the refactoring has left the functionality of dropq unchanged. When you force a test failure at the end of the final test_with_pufcsv test in the test_dropq.py file, you get the following output:

E       assert 1 == 2
tests/test_dropq.py:313: AssertionError
----------------------------- Captured stdout call -----------------------------
You loaded data for 2009.
Tax-Calculator startup automatically extrapolated your data to 2013.
You loaded data for 2009.
Tax-Calculator startup automatically extrapolated your data to 2013.
You loaded data for 2009.
Tax-Calculator startup automatically extrapolated your data to 2013.
You loaded data for 2009.
Tax-Calculator startup automatically extrapolated your data to 2013.
seed=3047708076
elapsed time for this run:  21.5277588367
f,d,adiff,pdiff=  2914.4330  2914.4160  0.0170  5.84454138733e-06

The seed value and the post-reform combined tax revenues without and with dropq are exactly the same as they are on the master branch.

@MattHJensen @feenberg @Amy-Xu @andersonfrailey @GoFroggyRun @codykallen @zrisher
@PeterDSteinberg @brittainhard

@codecov-io
Copy link

codecov-io commented Apr 26, 2017

Codecov Report

Merging #1314 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1314      +/-   ##
==========================================
- Coverage   99.65%   99.64%   -0.02%     
==========================================
  Files          38       38              
  Lines        2890     2800      -90     
==========================================
- Hits         2880     2790      -90     
  Misses         10       10
Impacted Files Coverage Δ
taxcalc/dropq/dropq_utils.py 100% <100%> (ø) ⬆️
taxcalc/dropq/__init__.py 100% <100%> (ø) ⬆️
taxcalc/dropq/dropq.py 100% <100%> (ø) ⬆️
taxcalc/taxcalcio.py 100% <0%> (ø) ⬆️
taxcalc/calculate.py 100% <0%> (ø) ⬆️

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 6ef4490...fc06a73. Read the comment docs.

@martinholmer
Copy link
Collaborator Author

Are there any comments on Tax-Calculator pull request #1314?

@MattHJensen @feenberg @Amy-Xu @andersonfrailey @GoFroggyRun @codykallen @zrisher
@PeterDSteinberg @brittainhard

@martinholmer martinholmer mentioned this pull request May 1, 2017
@martinholmer
Copy link
Collaborator Author

Commit fc06a73 in pull request #1314 removes the format_macro_results from the dropq.py file. This has been done because Tax-Calculator knows nothing about the internal variables being used in the OG-USA macro model. The appropriate place for that information is in OG-USA.

The create_json_table function has been moved from dropq_utils.py to dropq.py and "exported" via the dropq/__init__.py file. This means that the logic in the old format_macro_results function can be easily implemented by OG-USA or by TaxBrain.

@MattHJensen @feenberg @Amy-Xu @andersonfrailey @GoFroggyRun @codykallen @zrisher
@rickecon @jdebacker @PeterDSteinberg @brittainhard

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.

3 participants