Parameters class API redesign

[martinholmer]

This pull request includes a redesign of the Parameters class API plus Sphinx-aware documentation. The new API is as described in issue #336. The old Parameters class had no documentation, but a combination of the source code, the tests in test_parameters.py and test_calculate.py, and the helpful discussion in issue #342, provided a basis for the redesign. The redesign includes a replacement for the old global default_data function, which passes all the tests and works as described in the issue #342 discussion. Several changes have been made on Sep 1 (see details below in commit history) in response to helpful suggestions made by T.J. Alumbaugh (@talumbau) and @MattHJensen.

[talumbau]

same comment as above here, regarding 'raises' in the test name.

[martinholmer]

@MattHJensen I tried to test this in dropq and the web application. Here is what I found:

the tests in webapp-public pass in the one case where we call default_data using the new default_data

I was unable to test this branch in dropq because of a behavior change between the existing update method and the new implement_reform method. I don't know if this is an intentional change or not. If it is intentional, @martinholmer and I will need to walk through how one should create a Calculator instance and add reforms. Here is the new behavior vs. the old:

from taxcalc import Calculator, Records, Parameters

# User specified plan, start at 2013
params = Parameters(start_year=2013)
# setting up for reform for 2015
params.set_year(2014)
params.set_year(2015)
assert params.current_year == 2015
mods = {2015: {'_II_rt3': [0.31], '_II_rt4': [0.39]}}
params.implement_reform(mods)
# I would assume params.current_year is still 2015
#this fails because params.current_year goes back to 2013
assert params.current_year == 2015

dropq builds Parameter instances assuming that after implementing a reform (or "updating" in the old way) the current_year remains the same. So, I could impose a solution in my branch and try to get dropq to run. Should I do that?

T.J. @talumbau (and @MattHJensen) would something like the following work?

params = Parameters()
mods = {2015: {'_II_rt3': [0.31], '_II_rt4': [0.39]}}
params.implement_reform(mods)
params.set_year(2015)

[talumbau]

@martinholmer for inline conversations, would you mind keeping the conversation inline using the Add a Line Note button?

We have many inline conversations on this PR and if we use the main Conversation thread I think it will get too complicated.

[MattHJensen]

I believe this PR is ready to be merged based on the following analysis:

There are two outstanding lines of discussion that have not yet been fully addressed, but after conversing with @martinholmer on the phone this morning, I believe both of them ought to be dealt with in follow-on work. These two areas, and the reason for separating them from this PR, are as follows:

1. @talumbau raised several points about test names and test coverage (or overcoverage) which have not yet been addressed. The first and foremost reason for dealing with them separately is that they are not urgent and do not affect the existing functionality of the code. Second, I believe our test naming conventions relate to our ongoing conversation over coding style (coding style #30 (comment)); in particular, test naming conventions will come up when we move on to the second step of our coding style conversation:

   "After that, @martinholmer will open a separate issue that lists all of the classes of warnings that pylint generates and opens a discussion on which to keep or whether to use some alternative to pylint."

2. @talumbau and I raised a point about allowing the user to specify a JSON file of tax parameters other than params.json. This too is not urgent for our users, and can therefore -- from a timing perspective -- be dealt with later. Moreover, @martinholmer raised the point that we should be doing some checking when such a file is read to ensure that the user supplied JSON is valid, which we do not currently do on master. When we reintroduce this feature, we ought to include some of this checking.
   As @zrisher has said,

   users will make eventually make every usage error you can think of. : )

An additional reason for separating both these issues into follow on work is that they might both require quite a bit of discussion, and this PR is already quite cluttered.

@talumbau, if this analysis seems reasonable to you, could you go ahead and merge this PR?

[talumbau]

@MattHJensen OK, I'll merge this and create follow-on issues.