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

Don't index SALT cap (ID_AllTaxes_c) #2388

Merged
merged 9 commits into from
Dec 12, 2019
Merged

Conversation

MaxGhenis
Copy link
Contributor

Fixes #2387

@MaxGhenis
Copy link
Contributor Author

This is failing the TCJA round trip test:

E ValueError:
E Round-trip-reform and current-law-policy param values differ for:
E ID_AllTaxes_c in 2020 : rtr=[10000.0, 10000.0, 5000.0, 10000.0, 10000.0] clp=[10414.17, 10414.17, 5207.08, 10414.17, 10414.17]

Seems like TCJA is stuck with the indexed values, but I can't find how to change that (or in general how to change whether a value is indexed in a reform).

cc @MattHJensen in case you'd started working on this

@MattHJensen
Copy link
Contributor

MattHJensen commented Dec 12, 2019

@MaxGhenis, these lines need to be all be the same as this line.

The ceiling expires in 2026, so we have to specify all values out to 2026 in order to specify the 9e+99 in that year.

The indexed value only applies after the last specified parameter value, so in this case it only begins affecting parameter values in 2027 and doesn't end up making any difference given that we're already at 9e+99. It is good that you changed it, but the hardcoded parameter values through 2025 need to be changed as well.

I'm hoping to get the bug release out today or tomorrow, so if you don't think you'll have a chance to look at this again until after that, please let me know.

Thanks a lot for tackling this!

@codecov
Copy link

codecov bot commented Dec 12, 2019

Codecov Report

Merging #2388 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2388   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          13      13           
  Lines        2749    2749           
======================================
  Hits         2749    2749

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 4a5b71f...c6103b4. Read the comment docs.

@MaxGhenis
Copy link
Contributor Author

Thanks @MattHJensen, all set now.

@MaxGhenis
Copy link
Contributor Author

Odd, there was a conda error after updating pufcsv_agg_expect.csv. It doesn't appear to have anything to do with that update.

$ conda create -n taxcalc-dev python=$TRAVIS_PYTHON_VERSION;
Fetching package metadata ...An unexpected error has occurred.

Per conda/conda#3935 (comment) (3 years old) the recommended fix is to use

CONDA_SSL_VERIFY=false conda update pyopenssl

Anyone hit this before or know how to do it for Travis? If not I can ask on that issue.

…. done

Solving environment: ...working... failed with repodata from current_repodata.json, will retry with next repodata source. to setup_conda_environment.cmd
@MattHJensen
Copy link
Contributor

Thanks a lot, @MaxGhenis. This looks good to me. Good for me to merge from your end?

@MaxGhenis
Copy link
Contributor Author

Yep!

@MaxGhenis
Copy link
Contributor Author

MaxGhenis commented Dec 12, 2019

Sorry I wanted to amend a previously messed-up commit message, didn't expect it to re-trigger tests. No code has changed so tests should succeed soon.

@MaxGhenis
Copy link
Contributor Author

OK all set for real now!

@MattHJensen
Copy link
Contributor

Thanks, Max. Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SALT cap (ID_AllTaxes_c) should not be indexed to inflation
2 participants