Revise Parameters class code, documentation, and tests

[martinholmer]

The revisions add Sphinx-aware docstrings (with lengthy docstrings for the class constructor and
for the update method) and a new classmethod called default_inflation_rate. The revisions also
eliminate all but a few pylint warnings in parameters.py file.. Also, test_parameters.py has been
revised to use the new default_inflation_rate classmethod and to eliminate some (but nowhere
close to all) pylint warnings. These revisions leave the 75 items in py.test passing and also
produce exactly the same results_puf.csv file (which is the output written by the test.py script)
as before the changes in this pull request.

[talumbau]

Seems like some reasonable tests here, although it seems to me that this is testing the same functionality for various parameters. The maintenance burden goes up for each hard coded value that is tested (since it could change in the future), but if you think there is benefit in keeping all of the functions here, no problem. The right place for this file is taxcalc/tests/test_reform.py (or really anything starting with test_, with each of the functions prefaced with test_ so that py.test will run them each time.

[talumbau]

A major component of the changes here is a modification of update method on the Parameters class. There hasn't been enough written about what this method accomplishes and I discovered that some important features of the method as it currently exists in master are not tested in master. Therefore, no one would know about how these features worked, unless they happened to be very familiar with how the webapp works and how the dropq packages work, which is unlikely. So, this the beginning of what will likely be much more comprehensive documentation on this method.

To start, here is an example of what happens with Parameters objects in the dropq package, that must continue to work for dropq and the webapp to work.

A few features to note:

• The parameter value can be a sequence of values, indicating what the value of the parameter should be over a sequence of years. So, in the gist, this line:

    user_mods[2010]['_II_em'] = [5000, 6000]

says that _II_em will have a value of 5000 for the year 2010, and a value of 6000 for the year 2011.

• We build a Parameters object in a special way, starting with a year in the past, then incrementing to the year we desire to use for the updates. In dropq and the webapp, we start with the year 2013 and increment until we are at 2015. This was due to some technical reason that @MattHJensen told me about, the details of which escape me at the moment. The user input from the web starts from the year 2015. This means that this chunk of code in master:

                    if self.current_year > self.start_year:
                        cur_val = getattr(self, name)
                        offset = self.current_year - self.start_year
                        cur_val[offset:] = nval[num_years_to_skip:]

is important, because we will have a start_year of 2013, then some later year, like 2016, will be the current year. If the user_mods key was specified as, say, 2015, that means this variable:

num_years_to_skip=self.current_year - year

is equal to 1 (i.e. 2016 - 2015). So, we skip the first value in the provided sequence, (e.g. for user_mods[2010]['_II_em'] = [5000, 6000] we would skip 5000 and the right-hand side would be 6000. The left-hand side cur_val is just a name for whatever array we are currently dealing with. We also have to index in to it because we need to offset by the start_year for the Parameters object.

I'm not sure all of that makes sense, but at the very least, you can download that gist to a file at the top of your repo and you will have a valid test that you can test against to continue with this PR. This test should go into master immediately, since dependent packages won't work unless the functionality described in those tests works.

There are at least two good catches here in parameters.py and a great overall improvement in the documentation, so I think we definitely want much of what is here. I'm not sure how the resolution of #325 impacts everything here, but we might need to alter the interface of expand_array to make things more clear. I believe @MattHJensen gave you access to dropq so let me know if you want some pointers on how to probe into that for this issue.

[martinholmer]

T.J. (@talumbau) said:

Seems like some reasonable tests here, although it seems to me that this is testing the same
functionality for various parameters. The maintenance burden goes up for each hard coded
value that is tested (since it could change in the future), but if you think there is benefit in
keeping all of the functions here, no problem. The right place for this [reform_test.py] file is
taxcalc/tests/test_reform.py (or really anything starting with test_, with each of the functions
prefaced with test_ so that py.test will run them each time.

I think the complexity of the test is worth it because it revealed a bug in the Parameters update
method.

I will move this test to the taxcalc/tests/test_parameters.py file because it is testing the ability
of the Parameters class to specify a multi-year reform involving both 1D and 2D parameters.

Does that approach make sense?

[martinholmer]

Apart from the inflation-indexing test results being inconsistent with those that will be
generated after pull request #326 is committed to master, I think everything is now OK
with this pull request #321. If others on the project team argree, then the question is
how to proceed.

I suggest that if everybody is OK with pull request #326, then we should proceed
as follows: (1) commit #326 to master, (2) I will fix the test results in #321 to agree
with the new method of applying inflation rates, and (3) commit #321 to master.

Does this seem like a reasonable approach? If not, what would be a better approach?

[martinholmer]

@MattHJensen and TJ @talumbau,
Thanks for your emails about the private dropq package used in the TaxBrain webapp.
As far a I could see, the dropq code uses the Parameters class in a very simple way,
and therefore, none of the changes in pull request #321 would interfer with dropq logic.
Or, did I miss some kind of conflict that you have your eye on? It also seems to me that
the multi-year reform test that has now been integrated into the test_parameters.py file
is a sufficient test for dropq usage of the Parameters class. Do you see it the same
way? Or, am I missing something?

[martinholmer]

@MattHJensen said:

It might make sense to wait on fixing up #321 until TJ has had a chance
to open the test PR in case that will affect how you should fix #321, but
we'll know better whether that is the case once T.J. has a chance to post
the forthcoming details on that.

TJ @talumbau, when do you expect to post this new pull request? It would
help me schedule my time if had a sense of your schedule.

[talumbau]

probably by 8pm central time today

[MattHJensen]

@martinholmer, were you interested in being able to specify a multi year reform like in this snippet?

reform = {
    '_AMT_thd_MarriedS': {
        _c_cpi': False
        2015: [60000],
        2016: [65000]
    },
    '_EITC_c': {
        2016: [[900, 5000, 8000, 9000]],
        2017: [[950, 6000, 8000, 9000]],
        2018: [[950, 6000, 8000, 1000]]
    }
}

Perhaps you were waiting to discuss this as a separate issue later.

[talumbau]

OK, I've put up PR #330 that has a test in it that this PR will fail. The reason it fails is because this branch has a test that makes sure that there is only one year given in user_mods. That's probably reasonable for now. At least, our tests only use user_mods that way, so we should probably enforce that for now. However, calculator supports some special arguments, and, as a result of the merging operations that happen in the first few lines, you end up with final_mods having two items, one of which is an empty dictionary. The following line of code will remove it:

+    #Get rid of any empty dictionaries resulting from the merge:
+    final_mods = dict(filter(lambda x: len(x[1]) > 0,
+                            ((k,v) for k, v in final_mods.items())))

Put that in around line 35 or so in calculate.py and you will then pass the new test.

[martinholmer]

TJ @talumbau said:

OK, I've put up PR #330 that has a test in it that this PR will fail. The reason it fails is because this branch has a test that makes sure that there is only one year given in user_mods. That's probably reasonable for now. At least, our tests only use user_mods that way, so we should probably enforce that for now. However, calculator supports some special arguments, and, as a result of the merging operations that happen in the first few lines, you end up with final_mods having two items, one of which is an empty dictionary. The following line of code will remove it:

• #Get rid of any empty dictionaries resulting from the merge:
• final_mods = dict(filter(lambda x: len(x[1]) > 0,

•                        ((k,v) for k, v in final_mods.items())))
  

  Put that in around line 35 or so in calculate.py and you will then pass the new test.

This is a perfect example of the problems created by a lack of object-oriented desgin
in the OSPC Tax-Calculator. As I said to @MattHJensen and TJ @talumbau several
weeks ago, there is no reason for the Calculator class to be doing any work with a
Parameters object other than calling its increment method, at least as far as I can see.
Am I wrong about this? In particular, it is poor design to be implementing Parameter
reforms in the Calculator class; that is up to the Parameters class. We really need to
discuss the basic DESIGN (not just the coding style) of the taxcalc classes.

[martinholmer]

@MattHJensen said:

@martinholmer, were you interested in being able to specify a multi year reform like in this snippet?

reform = {
    '_AMT_thd_MarriedS': {
        _c_cpi': False
        2015: [60000],
        2016: [65000]
    },
    '_EITC_c': {
        2016: [[900, 5000, 8000, 9000]],
        2017: [[950, 6000, 8000, 9000]],
        2018: [[950, 6000, 8000, 1000]]
    }
}

Perhaps you were waiting to discuss this as a separate issue later.

Actually, the basic design of the Parameters class already allows the above, as
shown in the test_multi_year_reform() function in the test_parameters.py file
included in this pull request. Given that this is not a new feature, I did not think
it needed an discussion. But your question suggests that I'm missing something.
If so, what am I missing?

[talumbau]

I missed this before. What is the meaning of the comment here?

[talumbau]

This is a perfect example of the problems created by a lack of object-oriented desgin
in the OSPC Tax-Calculator. As I said to @MattHJensen and TJ @talumbau several
weeks ago, there is no reason for the Calculator class to be doing any work with a
Parameters object other than calling its increment method, at least as far as I can see.
Am I wrong about this? In particular, it is poor design to be implementing Parameter
reforms in the Calculator class; that is up to the Parameters class. We really need to
discuss the basic DESIGN (not just the coding style) of the taxcalc classes.

That sounds like a perfectly reasonable position, but I don't see how it applies to this situation. As I mentioned before, there is nothing here regarding a breaking of encapsulation, or really any OO concepts at all. There exists a free standing function called calculator inside of a module with a file name of calculate.py. It is not a method of the Calculator class. I had actually thought about calling this function build_calculator and putting it in utils.py. If you look at the implementation of the Calculator class, which begins on line 65 and ends at line 320 in master, there is no mutating access of the Parameters instance except for calls to the increment_year method when you are incrementing the year of the Calculator instance itself. The purpose of the free-standing function calculator was simply to reduce a bunch of typing and also allow a combination of keyword arguments along with a user_mods dictionary. That idea should stand or fall on its own merits. It's not an OO discussion.

[talumbau]

The setattr is not needed here. If you remove this line of code from the parameters-edit2 branch, all of the tests still pass. cval is just a name associated with whatever attribute we are modifying. Line 238 does the mutation of that value. After that, there is no more work to do.

[talumbau]

The last issue that should be discussed with this PR is that is puts a particular restriction on the user reform year to be the same exact year as the current year for the Parameters instance. The implementation of the update method is simpler because of this, but accepting this PR means that we could no longer due something like this. The idea of that gist is that the user_mods specifies 2011 behavior (by providing a sequence of values) but the first year given is 2010. We then update a Parameters instance that has already advanced to 2011. In master, this is allowed. The Parameters instance, with current_year==2011 will select out the later values (in this case, just the values for 2011). In this branch, the behavior is disallowed.

This is just a trade-off. The implementation of update in this branch is a little shorter and perhaps people will find it easier to use and understand. In master, we provide the flexibility shown in the gist. I would feel most comfortable if @MattHJensen would sign off on the choice implied here. Should we be able to do what I describe above in the gist (as in master) or should this capability raise an error (as in this branch)?

[MattHJensen]

This is a perfect example of the problems created by a lack of object-oriented desgin
in the OSPC Tax-Calculator. As I said to @MattHJensen and TJ @talumbau several
weeks ago, there is no reason for the Calculator class to be doing any work with a
Parameters object other than calling its increment method, at least as far as I can see.
Am I wrong about this? In particular, it is poor design to be implementing Parameter
reforms in the Calculator class; that is up to the Parameters class. We really need to
discuss the basic DESIGN (not just the coding style) of the taxcalc classes.

Yes, @martinholmer, I definitely agree that we should be continually thinking about how we can make improvements to the code base, and your suggestions on that front would be much appreciated. The way I'd suggest going about this would be to set up a video conference/screenshare to make sure we're all an the same page about the existing taxcalc code base as well as the codebase in dropq and webapp. Then you can make your suggestions via an issue + gist or via a PR (the choice of these two paths would depend on how likely you think everyone else is to agree with your suggestions, how much work it is to implement, and whether it is easy to communicate your intent via a pared down example)

[MattHJensen]

@martinholmer, were you interested in organizing these dictionaries by parameter rather than by year. I.e, like the snippet below rather than what is shown in the test?

reform = {
    '_AMT_thd_MarriedS': {
        _c_cpi': False
        2015: [60000],
        2016: [65000]
    },
    '_EITC_c': {
        2016: [[900, 5000, 8000, 9000]],
        2017: [[950, 6000, 8000, 9000]],
        2018: [[950, 6000, 8000, 1000]]
    }
}

This would parallel more closely, (and potentially be a better supplement, even alternative to) our current method of specifying multi-year reforms {name: [1st_yr_val, 2nd_yr_val, 3rd_yr_val, ...]}

[MattHJensen]

This is just a trade-off. The implementation of update in this branch is a little shorter and perhaps people will find it easier to use and understand. In master, we provide the flexibility shown in the gist. I would feel most comfortable if @MattHJensen would sign off on the choice implied here.

Let me dig into this later today after I get the pep8 PR tied up.

[martinholmer]

This pull request #321 is now not able to be merged into the master branch because
of the many changes to master that have been made since this branch was started
and because of the extensive changes in master that will be made in the next day or
so. And, more importantly, this pull request has generated quite a few doubts and
concerns about its wisdom. Given all this, it seems best to withdraw this pull request.

[MattHJensen]

I'm still hoping the vast majority of this content gets merged. Doesn't hurt to wait until after our code design discussion to move forward, though.