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

Remove direct reliance on PUF and CPS files: Stage 1 (PUF) #2538

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

chusloj
Copy link
Contributor

@chusloj chusloj commented Jan 29, 2021

This PR is the first in a pair of PRs that will remove Tax-Calculator's direct reliance on puf.csv and cps.csv files. This PR focuses on all content regarding the PUF, and the next will focus on the CPS file.

Each PR will be updated in stages:

  1. Functions (and docstrings) and the CLI
  2. Testing
  3. Documentation

@chusloj chusloj marked this pull request as draft January 29, 2021 19:31
@chusloj chusloj changed the title Remove puf functions Remove direct reliance on PUF and CPS files: Stage 1 (PUF) Jan 29, 2021
@chusloj chusloj changed the title Remove direct reliance on PUF and CPS files: Stage 1 (PUF) [WIP] Remove direct reliance on PUF and CPS files: Stage 1 (PUF) Feb 1, 2021
@chusloj chusloj changed the title [WIP] Remove direct reliance on PUF and CPS files: Stage 1 (PUF) [Matt please see] Remove direct reliance on PUF and CPS files: Stage 1 (PUF) Feb 1, 2021
@chusloj chusloj changed the title [Matt please see] Remove direct reliance on PUF and CPS files: Stage 1 (PUF) [Matt please check] Remove direct reliance on PUF and CPS files: Stage 1 (PUF) Feb 1, 2021
@chusloj chusloj changed the title [Matt please check] Remove direct reliance on PUF and CPS files: Stage 1 (PUF) [WIP] Remove direct reliance on PUF and CPS files: Stage 1 (PUF) Feb 2, 2021
@chusloj
Copy link
Contributor Author

chusloj commented Feb 2, 2021

Just a small note, the only change this will make to the Tax-Calculator API is that users will be required to specify the path to their data, weights, and adjust_ratios. They'll also be required to provide the start_year for their data.

Currently, if a user has the PUF file, a Records object would just be created as:

recs = Records()

Now, the user has to specify the 4 variables mentioned above:

puf_data = 'puf_data.csv'
puf_weights = 'puf_weights.csv'
puf_ratios = 'puf_ratios.csv'
start_year = 2012

recs = Records(data=puf_data, weights=puf_weights, adjust_ratios=puf_ratios, start_year=start_year)

@chusloj
Copy link
Contributor Author

chusloj commented Feb 10, 2021

The test suite has been updated. PUF data and related files have been replaced with synthetic data in tests. test_compare, test_puf_var_stats and test_pufcsv will be moved to TaxData once this PR closes. Also, test_compatabile_data will be moved to Tax-Brain.

@chusloj
Copy link
Contributor Author

chusloj commented Feb 12, 2021

The CLI, docs for the CLI and recipes have been updated to support the new API.

@chusloj chusloj marked this pull request as ready for review February 12, 2021 21:41
@chusloj chusloj changed the title [WIP] Remove direct reliance on PUF and CPS files: Stage 1 (PUF) [Review Ready] Remove direct reliance on PUF and CPS files: Stage 1 (PUF) Feb 12, 2021
@chusloj
Copy link
Contributor Author

chusloj commented Feb 18, 2021

A script to generate random test data has been included to provide test data to replace the PUF. The generated data supplements data produced by validation set 'c' from the TAXSIM32 PR.

All tests are passing. AppVeyor is failing because the test data generation script is only run in the GH action.

@MattHJensen MattHJensen reopened this Feb 24, 2021
@chusloj
Copy link
Contributor Author

chusloj commented Feb 26, 2021

As a check, I'll test the TAXSIM32 suite using the new API/CLI once that PR is merged.

@chusloj
Copy link
Contributor Author

chusloj commented Apr 29, 2021

The only failing test here is a difference between reforms_actual.csv and reforms_expect.csv. Locally however, many tests fail (most in test_reforms.py) with the following error:

 IndexError: positional indexers are out-of-bounds

This might be related to a newly updated dependency either in taxcalc or paramtools affecting how JSON files are treated. The same dependency issues might be affecting tests in #2588 as well.

@MattHJensen MattHJensen changed the title [Review Ready] Remove direct reliance on PUF and CPS files: Stage 1 (PUF) [WIP] Remove direct reliance on PUF and CPS files: Stage 1 (PUF) May 4, 2021
@martinholmer martinholmer changed the title [WIP] Remove direct reliance on PUF and CPS files: Stage 1 (PUF) Remove direct reliance on PUF and CPS files: Stage 1 (PUF) May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants