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.

Sign up for GitHub

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

Jump to bottom

Split UBI_21 into UBI_2164 and UBI_65 [3.0] #2463

Merged
merged 14 commits into from
Aug 20, 2020
Merged

Split UBI_21 into UBI_2164 and UBI_65 [3.0] #2463

merged 14 commits into from
Aug 20, 2020

Conversation

MaxGhenis
Copy link
Contributor

Fixes #2451

@MaxGhenis
Copy link
Contributor Author

I thought I replaced UBI_21 with UBI_2164 everywhere it was present, but I'm still getting AttributeError: UBI_2164 not defined. from the bottom of parameters.py. Could someone advise?

@MaxGhenis
Copy link
Contributor Author

Ah it's policy_current_law.json. GitHub search will never cease to disappoint me. Adding now.
image

@codecov
Copy link

codecov bot commented Aug 20, 2020 •
edited
Loading

Codecov Report

Merging #2463 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2463   +/-   ##
=======================================
  Coverage   99.92%   99.92%           
=======================================
  Files          13       13           
  Lines        2582     2582           
=======================================
  Hits         2580     2580           
  Misses          2        2
Impacted Files Coverage Δ
taxcalc/policy.py 100.00% <ø> (ø)
taxcalc/parameters.py 99.33% <100.00%> (ø)

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 5cb7504...2397d82. Read the comment docs.

@MaxGhenis MaxGhenis changed the title [WIP] Splits UBI_21 into UBI_2164 and UBI_65 Split UBI_21 into UBI_2164 and UBI_65 Aug 20, 2020
@MaxGhenis
Copy link
Contributor Author

This is now ready. It is backwards-incompatible so should only be merged for 3.0.

@MaxGhenis
Copy link
Contributor Author

MaxGhenis commented Aug 20, 2020 •
edited
Loading

Also I ran make_uguide.py so the fields differ. I can revert those changes, or if it's merged close to the 3.0 release we could leave it then, LMK @MattHJensen.

@MattHJensen MattHJensen changed the title Split UBI_21 into UBI_2164 and UBI_65 Split UBI_21 into UBI_2164 and UBI_65 [3.0] Aug 20, 2020
@MattHJensen
Copy link
Contributor

MattHJensen commented Aug 20, 2020 •
edited
Loading

Thanks @MaxGhenis! Will review soon, and I also asked @Peter-Metz in chat if he could take a look given his recent experience deprecating params. I just added a 3.0 tag to remind us this needs to go in before the release.

@Peter-Metz
Copy link
Contributor

Hi @MaxGhenis this looks good to me. My only suggestion would be to add UBI_21 to the list of removed_params. This prompts a more helpful error messages when someone tries to use a deprecated param

hdoupe
hdoupe reviewed Aug 20, 2020
View reviewed changes
taxcalc/parameters.py
@@ -737,4 +737,4 @@ def __getattr__(self, attr):
attr[1:], year=list(range(self.start_year, self.end_year + 1))
)
else:
raise AttributeError(f"{attr} not definied.")
raise AttributeError(f"{attr} not defined.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, thanks @MaxGhenis!

@MattHJensen MattHJensen changed the title Split UBI_21 into UBI_2164 and UBI_65 [3.0] [WIP] Split UBI_21 into UBI_2164 and UBI_65 [3.0] Aug 20, 2020
@MattHJensen
Copy link
Contributor

Added the WIP tag for resolution of @Peter-Metz's suggestion (Thanks Peter!). @MaxGhenis, please just drop it when you're ready.

@MaxGhenis MaxGhenis changed the title [WIP] Split UBI_21 into UBI_2164 and UBI_65 [3.0] Split UBI_21 into UBI_2164 and UBI_65 [3.0] Aug 20, 2020
@MaxGhenis
Copy link
Contributor Author

Thanks for the suggestion, @Peter-Metz, I added UBI_21 to the list of removed params. Ready to merge now.

@MattHJensen
Copy link
Contributor

Thanks @MaxGhenis! Much appreciated. Merging now.

@MattHJensen MattHJensen merged commit 3ab1d20 into PSLmodels:master Aug 20, 2020
@Peter-Metz Peter-Metz mentioned this pull request Aug 21, 2020
Open
@MattHJensen MattHJensen mentioned this pull request Aug 21, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hdoupe hdoupe hdoupe left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Separate UBI parameters for 21-64 and 65+
4 participants
@MaxGhenis @MattHJensen @Peter-Metz @hdoupe