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

CBO baseline update #2662

Merged
merged 12 commits into from Dec 14, 2022
Merged

CBO baseline update #2662

merged 12 commits into from Dec 14, 2022

Conversation

bodiyang
Copy link
Contributor

@bodiyang bodiyang commented Jul 29, 2022

This PR serves as a follow up work after Tax-Data CBO baseline update.

New input data files will be updated in Tax-Calculator to match the update in Tax-Data

@codecov
Copy link

codecov bot commented Aug 9, 2022

Codecov Report

Merging #2662 (c2ac9fb) into master (98b23dc) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2662   +/-   ##
=======================================
  Coverage   98.54%   98.54%           
=======================================
  Files          14       14           
  Lines        2609     2609           
=======================================
  Hits         2571     2571           
  Misses         38       38           
Flag Coverage Δ
unittests 98.54% <100.00%> (ø)

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

Impacted Files Coverage Δ
taxcalc/policy.py 100.00% <100.00%> (ø)
taxcalc/utils.py 97.79% <100.00%> (ø)

@bodiyang bodiyang marked this pull request as draft August 9, 2022 19:21
@MattHJensen MattHJensen marked this pull request as ready for review August 29, 2022 17:06
@jdebacker
Copy link
Member

@bodiyang, when I run the unit tests on this branch on my machine, I get the following failures:

FAILED taxcalc/tests/test_calculator.py::test_itemded_component_amounts[2017-c18300-ID_AllTaxes_hc]
FAILED taxcalc/tests/test_calculator.py::test_itemded_component_amounts[2017-c19700-ID_Charity_hc]
FAILED taxcalc/tests/test_compare.py::test_itax_compare[True] - ValueError: C...
FAILED taxcalc/tests/test_puf_var_stats.py::test_puf_var_stats - ValueError: ...
FAILED taxcalc/tests/test_pufcsv.py::test_agg - ValueError: PUFCSV AGG RESULT...
ERROR taxcalc/tests/test_utils.py::test_table_columns_labels - ValueError: AC...
== 5 failed, 346 passed, 44 skipped, 1 warning, 1 error in 1517.22s (0:25:17) ==

The PUF tests are not run on GH Actions. Are you getting everything to pass on your machine?

@bodiyang
Copy link
Contributor Author

@jdebacker all tests have been passed locally, including the PUF tests. However, the online built in tests failed now (before this commit, the tests were passing online).

@jdebacker
Copy link
Member

@bodiyang Thanks for the latest commits. Test failures seem to be related to an issue in the latest release of bokeh (see here). Unfortunately, I don't see a solution yet.

@jdebacker
Copy link
Member

@bodiyang Have you pushed all your changes to this branch? I synced and ran tests again locally and still found failures.

@bodiyang
Copy link
Contributor Author

bodiyang commented Dec 2, 2022

@jdebacker yes I have pushed all changes

56708fb
Screen Shot 2022-12-01 at 7 09 47 PM

@jdebacker
Copy link
Member

@bodiyang Thanks for confirming. I created a new PUF from the most recent taxdata and then ran the unit test. All test, except for those using bokeh passed, confirming your results.

My last comment then, is that you should update the cps.csv.gz file with the the version produced from the most recent taxdata (and any associated files that should be updated) with this PR (e.g., as was done in PR #2599).

@bodiyang
Copy link
Contributor Author

bodiyang commented Dec 6, 2022

@bodiyang Thanks for confirming. I created a new PUF from the most recent taxdata and then ran the unit test. All test, except for those using bokeh passed, confirming your results.

My last comment then, is that you should update the cps.csv.gz file with the the version produced from the most recent taxdata (and any associated files that should be updated) with this PR (e.g., as was done in PR #2599).

I've run the make all and make cps-files commend in Tax-Data, to generate all cps files. However only cps_raw.csv.gz is updated, while cps.csv.gz is not updated. Checked stage-II.py, there is no code do the update for cps.csv.gz.
Then I try to run createcps.py script, still trying to generate the cps.csv.gz file. But there is no change made to the cps.csv.gz file ~ the time of modify of the file looks changed, but the file itself is not. So possibly the cps.csv.gz file has no update from this CBO baseline update. Any other possibilities?

@jdebacker
Copy link
Member

@bodiyang Ok, I confirmed my cps.csv.gz produced by taxdata:master gives exactly the same data as what is in the cps.csv.gz file in Tax-Calculator.

In tax data PR #412, @andersonfrailey notes that it was odd that the CPS file did not change, but could not identify any incorrect steps in that PR so it was merged anyway.

Thus, I suppose it's ok that there are no changes to the cps.csv.gz file.

@jdebacker
Copy link
Member

@bodiyang I just opened a PR to your CBO-baseline branch which should take care of the test failures here. Please give it a look and merge it in and we'll see if that's all we need.

@jdebacker
Copy link
Member

@bodiyang Thanks or merging my PR. It seems there is still one test failure when using bokeh >= 3.0.0 with a serialization error. I found an issue related to that, but it seems to suggest it's been resolved. I have unsuccessfully tried to find a work around in our case.

Let's leave this sit for another day or so and if nothing turns up, perhaps we just pin to bokeh < 3.0.0 in the environment.yml file.

@jdebacker
Copy link
Member

@bodiyang Can you change environment.yml to replace - "bokeh>=1.4.0, <3.0.0"

@bodiyang
Copy link
Contributor Author

@bodiyang Can you change environment.yml to replace - "bokeh>=1.4.0, <3.0.0"

Have updated the package, now passed all tests with PUF locally and also the online testing

@jdebacker
Copy link
Member

LGTM - thanks @bodiyang!

@jdebacker jdebacker merged commit cfed02e into PSLmodels:master Dec 14, 2022
@bodiyang bodiyang deleted the CBO-update branch December 14, 2022 15:54
@bodiyang bodiyang restored the CBO-update branch December 14, 2022 16:35
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.

None yet

2 participants