PSLmodels / Business-Taxation

USA Corporate and Pass-Through Business Tax Model
11 stars 10 forks source link

Use taxcalc.Parameters._validate_* methods instead of local methods #88

Closed martinholmer closed 5 years ago

martinholmer commented 5 years ago

This pull request takes advantage of Tax-Calculator enhancements forthcoming in release 2.0.0 to simplify the Business-Taxation Policy class. There are no changes in logic, just a reduction in the amount of code in the Policy class. The code reduction is possible because now the Business-Taxation Policy class, which is a specialized Parameters class, can rely on the new forthcoming capabilities of the Parameters class.

martinholmer commented 5 years ago

@codykallen, wait until packages for the next version of Tax-Calculator are available before merging PR#88.

martinholmer commented 5 years ago

@codykallen, Now that packages for Tax-Calculator release 2.0.0 are available, Business-Taxation pulll request #88 is ready for your review.

codykallen commented 5 years ago

@martinholmer, these changes look good to me.

In terms of future development, are there any plans to switch from taxcalc.Parameters logic to paramtools?

martinholmer commented 5 years ago

@codykallen asked in PR #88:

In terms of future development, are there any plans to switch from taxcalc.Parameters logic to paramtools?

I'm not sure what such a change would provide as benefits to either developers or users of Business-Taxation. My understanding is that such a change would impose costs. Developers would have to think about two parameter frameworks instead of one. And users would have to remember a different way of specifying individual tax reforms and business tax reforms.

hdoupe commented 5 years ago

@martinholmer said:

My understanding is that such a change would impose costs. Developers would have to think about two parameter frameworks instead of one. And users would have to remember a different way of specifying individual tax reforms and business tax reforms.

If there's interest in using ParamTools, I'm happy to make changes to make the experience between taxcalc.Parameters and Paramtools as seamless as possible. I've been thinking of ways to make the adjustments files compatible with taxcalc reform files. It would be a straightforward translation of:

{
    "param": [{"year": 2020, "value": 0.1}]
}

to

{
    "param": {"2020": 0.1}
}

The developer experience should already be seamless. For example, ParamTools is a drop in replacement for CCC's parameters class.

ParamTools is also able to do parameter indexing.

hdoupe commented 5 years ago

Just to clarify, the point in my comment above (https://github.com/PSLmodels/Business-Taxation/pull/88#issuecomment-483673260) is that ParamTools will soon be interchangeable in some regards with taxcalc.Parameters. This will make it easy to swap over to ParamTools in the future, if you are interested in doing so.