Use ParamTools #2401

hdoupe · 2020-03-02T15:45:22Z

This PR shows how ParamTools could be used to handle Tax-Calculator's parameters. I used a script to convert the configuration files from the Tax-Calculator format to the ParamTools format, but besides this difference, there are only a couple public-facing API changes:

The inflation_rates() and growth_rates() methods now return dictionaries where the keys are years and the values are the rates. It was easier for me to work with this format since most operations require you to get a value for a certain year at a time. This can easily be reverted.

The parameter values that are accessible as instance attributes (e.g. pol.II_em) are not casted down into a lower dimension. So, if the year is 2017, then pol.II_em returns [4050.0] instead of 4050.0. ParamTools does this to keep the results consistent for when you have pol.set_year(2017) or pol.set_year([2017, 2018]). The Calculator.policy_param method automatically casts the value into a lower dimension to preserve compatibility:

In [2]: pol = Policy()                                                                                                                                                                        

In [3]: pol.set_year(2017)                                                                                                                                                                    

In [4]: pol.II_em                                                                                                                                                                             
Out[4]: array([4050.])

In [5]: pol.set_year([2017, 2018])                                                                                                                                                            

In [6]: pol.II_em                                                                                                                                                                             
Out[6]: array([4050.,    0.])

In [7]: from taxcalc import Policy, Calculator, Records                                                                                                                                       

In [8]: calc = Calculator(policy=pol, records=Records.cps_constructor())                                                                                                                      

In [9]: calc.policy_param("II_em")                                                                                                                                                            
Out[9]: 4050.0

There may be some other API changes that I have missed. I'll be going over the code to make sure that I have not missed anything else. If anyone is interested, I'd really appreciate them running their downstream projects or notebooks with the changes in this branch to make sure I haven't broken anything.

There are few notable places where I maintained consistency:

The _update method still takes reforms that are in the Tax-Calculator format. The reforms are reformatted and passed to the ParamTools adjust method. All classes that inherit from taxcalc.Parameters will have this behavior. That's why none of the reforms in this repo needed to be reformatted in this PR.

Getting the parameter value using a leading underscore still returns its values for all years:

In [10]: pol._II_em                                                                                                                                                                           
Out[10]: 
array([3900.  , 3950.  , 4000.  , 4050.  , 4050.  ,    0.  ,    0.  ,
          0.  ,    0.  ,    0.  ,    0.  ,    0.  ,    0.  , 4901.  ,
       5004.41, 5108.5 , 5215.78])

This is done with a custom attribute getter which checks if the attribute meets this pattern and then does the necessary operations to get the values for all years.

Other attributes and methods like start_year, current_year, and set_year were either copied over to the new Parameters class or re-written to wrap the ParamTools API to maintain consistency.

I opened this PR as a draft because I'm not sure if the Tax-Calculator project is interested in adopting ParamTools. After all, Tax-Calculator has a perfectly good parameters processing backend that is fast, reliable, and well-tested. This exercise was very helpful for ParamTools. It prompted me to improve ParamTools's performance, make it more extendable, add better error messages, add warnings, and the list goes on. I even spotted a small bug in Tax-Calculator (#2381) while I was re-implementing the inflation indexing algorithm with ParamTools. I also included detailed documentation on the indexing algorithm in the new adjust method, which could be helpful to have in the Tax-Calculator docs. So, I think this exercise was helpful for Tax-Calculator, too. I'm excited to hear people's feedback on this PR.

… keep api compat

…'s a Policy parameter - Updates test_decorators.py for change in Policy parameter value dimensionality

…r all years

…asier

…mTools

…oved/renamed parameters

hdoupe · 2020-03-02T16:08:57Z

The test errors look like pycodestyle errors. I'll push some fixes soon.

hdoupe · 2020-03-04T15:41:01Z

I just pushed a whole bunch of codestyle fixes. I've been spoiled by my use of tools that format code for me. One thing that might be helpful for TC is to add a pre-commit hook that runs the pycodestyle command when you create a git commit (for example). If the code style test fails, then it won't let you make the commit.

hdoupe · 2020-03-04T16:01:54Z

codecov · 2020-03-04T16:07:09Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #2401   +/-   ##
=======================================
  Coverage   99.92%   99.92%           
=======================================
  Files          13       13           
  Lines        2530     2530           
=======================================
  Hits         2528     2528           
  Misses          2        2

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 a892c37...a892c37. Read the comment docs.

Peter-Metz · 2020-04-27T19:34:02Z

@hdoupe I reviewed the changes to Tax-Calc tests and they look good to me! (I didn't check the tests that are now handled by ParamTools)

@MattHJensen

hdoupe · 2020-04-27T20:03:43Z

Thanks @Peter-Metz. Per @MattHJensen's suggestion I'm going to add a ParamTools version of the array tests.

I'm going to have time in about an hour to try to knock out these tasks per your feedback:

Fix key order in JSON files
Add back print_warnings functionality
Add array tests

Let me know if I'm missing anything.

hdoupe · 2020-04-27T20:05:46Z

@MattHJensen thanks for your feedback on how warnings should work in Tax-Calculator. I'm going to implement your suggestion in this PR.

Peter-Metz · 2020-04-27T21:02:20Z

@hdoupe the only other thing i can think of is fixing the user guide. Let me know if I can help with any of these tasks.

hdoupe · 2020-04-27T21:09:05Z

@hdoupe the only other thing i can think of is fixing the user guide. Let me know if I can help with any of these tasks.

Do you have time to look at the user guide? I modified the make_uguide.py file so that it runs, but I haven't looked at the output closely yet.

Peter-Metz · 2020-04-27T22:29:22Z

@hdoupe Yep, I'll take a look tomorrow.

jdebacker · 2020-04-30T03:17:38Z

I installed TC from this branch and successfully ran all tests of the latest version of Cost of Capital Calculator with it.

@rickecon or I will try with OG-USA soon.

cc @hdoupe @MattHJensen @Peter-Metz

hdoupe · 2020-04-30T13:12:36Z

Thanks @jdebacker! This is much appreciated!

- Adds back expand_array tests - Use to_array labels argument in '_{attr}' logic - Move initialize logic to __init__ method but keep initialize for legacy purposes

hdoupe · 2020-05-01T20:10:46Z

Thanks @Peter-Metz. Per @MattHJensen's suggestion I'm going to add a ParamTools version of the array tests.

I'm going to have time in about an hour to try to knock out these tasks per your feedback:
* [x]  Fix key order in JSON files

* [x]  Add back `print_warnings` functionality

* [x]  Add array tests
Let me know if I'm missing anything.

These tasks are complete. @MattHJensen @Peter-Metz let me know how things look.

Here's the script I used to fix the key order:

import json

if __name__ == "__main__":
    to_convert = [
        "taxcalc/policy_current_law.json",
        "taxcalc/growdiff.json",
        "taxcalc/consumption.json",
    ]

    keys = [
        "title",
        "description",
        "notes",
        "section_1",
        "section_2",
        "section_3",
        "start_year",
        "indexable",
        "indexed",
        "type",
        "value",
        "validators",
        "compatible_data",
    ]

    for path in to_convert:
        print('converting', path)
        with open(path, "r") as f:
            defaults = json.loads(f.read())

        sorted_defaults = {}
        if "schema" in defaults:
            sorted_defaults["schema"] = defaults["schema"]

        for param, data in defaults.items():
            if param == "schema":
                continue
            assert not (data.keys() - set(keys)), f"{param} has extra keys: {data.keys()}"
            sorted_defaults[param] = {}
            for key in keys:
                if key in data:
                    sorted_defaults[param][key] = data[key]

        with open(path, "w") as f:
            f.write(json.dumps(sorted_defaults, indent=4))

Reformat user guide

jdebacker · 2020-05-02T13:00:05Z

@hdoupe I ran through OG-USA with TC built from this branch after commit f2c1dbd and has no issues.

cc @MattHJensen @Peter-Metz

hdoupe · 2020-05-03T13:58:41Z

Glad to hear it @jdebacker. Thanks again for testing out these changes.

hdoupe · 2020-05-04T15:59:28Z

@Peter-Metz @MattHJensen #2401 is ready for another round of review. I think we should be pretty close to getting this merged.

@Peter-Metz are you still good with re-working the handling of the CPI_offset parameter in a follow up PR?

Peter-Metz · 2020-05-04T18:03:21Z

@hdoupe @MattHJensen changes look good to me and print_warnings functionality works as desired!

Hank - yeah, I have the CPI_offset fix pretty much teed up as soon as this PR is merged.

hdoupe · 2020-05-04T19:04:44Z

@Peter-Metz sounds great. I'm excited to see your CPI_offset work.

MattHJensen · 2020-05-07T13:13:52Z

@hdoupe, thanks very much for this PR and for so generously accommodating Peter and my requests through the review process. I am merging this PR now and expect it to appear in a new release within the month.

MattHJensen · 2020-05-07T13:14:11Z

@Peter-Metz, thanks for all of your help with the review!

hdoupe · 2020-05-07T13:21:15Z

thanks very much for this PR and for so generously accommodating Peter and my requests through the review process

It was my pleasure. @MattHJensen and @Peter-Metz thanks for the thorough review!

hdoupe added 22 commits November 19, 2019 13:28

checpoint: most policy, calculator tests pass

73b5ede

Add paramtools to environment.yaml

6000bc4

Add warning handling

f41ec08

Add docstring to base parameters class and clean up args

16b0ee6

Update tests in test_parameters.py

d9e5428

Fix terminology in comment

c50e8d0

Remove debugging print statement from calcfunctions

d9f7c93

Revert inflation_rates and wage_growth_rates back to being methods to…

8217150

… keep api compat

Use pm/pf terminology instead of paremeter_list for determining if it…

5cd798a

…'s a Policy parameter - Updates test_decorators.py for change in Policy parameter value dimensionality

Fix missed inflation_rates to method reversion

88bde60

Progress on reform docs

058f4a4

Parameters is not iterable and use leading underscore to get value fo…

7605b25

…r all years

Catch paramtools.ValidationErrors instead of ValueErrors in taxcalcio

2e464a3

Fix documentation tests for growdiff.py

c8e13c1

Updata consumption tests for paramtools format and validation error

af6b64c

Swap back to old style json files to make merge conflict resolution e…

ab5ba93

…asier

Merge in TC 2.7.0 and 2.8.0

4972a33

Comment out code for testing _expand_array since it is tested in Para…

9040601

…mTools

Fix cpi offset bug reported in PSLmodels#2381

d5a4e81

Update tests for 2017 law and tcja round trip. Also try to handle rem…

9afeb46

…oved/renamed parameters

Re-write paramtools versions of json files

c0a3e81

Update pt version and add to packages test

98e3fe5

Fix a whole bunch of codestyle errors

6b00c61

Fix code-style

d282f5a

hdoupe added 2 commits March 4, 2020 19:04

Set up data in initialize method

59c90ed

Add start year to initial state to prevent extra set_state call

9820412

Add back print_warnings argument

c44dfbb

Peter-Metz and others added 6 commits April 30, 2020 09:35

remake uguide to resemble old tax-calc format

5cc1c2e

Updates for forthcoming paramtools release

65f5083

- Adds back expand_array tests - Use to_array labels argument in '_{attr}' logic - Move initialize logic to __init__ method but keep initialize for legacy purposes

Fixes to improve git diff

756a8f1

Update for better git diff in test_parameters expand tests

dd5cb89

Update paramtools requirement to 0.14.0

e32346c

Fix key order in defaults json files

437c33c

Merge pull request #2 from hdoupe/uguide

f2c1dbd

Reformat user guide

Remove extra set_state calls in _update

2c9aab7

hdoupe added 3 commits May 4, 2020 12:00

Fix docstring

fa1103e

Remove ctc_c warnings

17adb96

Update test for inflation/wage growth rate accesser methods

a892c37

MattHJensen merged commit 72f5de9 into PSLmodels:master May 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ParamTools #2401

Use ParamTools #2401

hdoupe commented Mar 2, 2020 •

edited

Loading

hdoupe commented Mar 2, 2020

hdoupe commented Mar 4, 2020

hdoupe commented Mar 4, 2020 •

edited

Loading

codecov bot commented Mar 4, 2020 •

edited

Loading

Peter-Metz commented Apr 27, 2020

hdoupe commented Apr 27, 2020 •

edited

Loading

hdoupe commented Apr 27, 2020

Peter-Metz commented Apr 27, 2020

hdoupe commented Apr 27, 2020

Peter-Metz commented Apr 27, 2020

jdebacker commented Apr 30, 2020

hdoupe commented Apr 30, 2020

hdoupe commented May 1, 2020

jdebacker commented May 2, 2020

hdoupe commented May 3, 2020

hdoupe commented May 4, 2020

Peter-Metz commented May 4, 2020 •

edited

Loading

hdoupe commented May 4, 2020

MattHJensen commented May 7, 2020

MattHJensen commented May 7, 2020

hdoupe commented May 7, 2020

Use ParamTools #2401

Use ParamTools #2401

Conversation

hdoupe commented Mar 2, 2020 • edited Loading

hdoupe commented Mar 2, 2020

hdoupe commented Mar 4, 2020

hdoupe commented Mar 4, 2020 • edited Loading

codecov bot commented Mar 4, 2020 • edited Loading

Codecov Report

Peter-Metz commented Apr 27, 2020

hdoupe commented Apr 27, 2020 • edited Loading

hdoupe commented Apr 27, 2020

Peter-Metz commented Apr 27, 2020

hdoupe commented Apr 27, 2020

Peter-Metz commented Apr 27, 2020

jdebacker commented Apr 30, 2020

hdoupe commented Apr 30, 2020

hdoupe commented May 1, 2020

jdebacker commented May 2, 2020

hdoupe commented May 3, 2020

hdoupe commented May 4, 2020

Peter-Metz commented May 4, 2020 • edited Loading

hdoupe commented May 4, 2020

MattHJensen commented May 7, 2020

MattHJensen commented May 7, 2020

hdoupe commented May 7, 2020

hdoupe commented Mar 2, 2020 •

edited

Loading

hdoupe commented Mar 4, 2020 •

edited

Loading

codecov bot commented Mar 4, 2020 •

edited

Loading

hdoupe commented Apr 27, 2020 •

edited

Loading

Peter-Metz commented May 4, 2020 •

edited

Loading