user_mods dict is a later year then start year

[talumbau]

• An additional test that is relevant for a production case
  First, make a Records and Parameters object at a given year,
  then increment, then update with a user_mods dict at some
  future year.

[talumbau]

this reveals a subtle bug in the calculator method that is revealed when running this test against the branch for PR #321. This test should probably go to master, then the fix for the bug in calculator should also be put in the same branch as that PR.

[talumbau]

cc @martinholmer @MattHJensen

[martinholmer]

TJ @talumbau said:

An additional test that is relevant for a production case.
First, make a Records and Parameters object at a given year,
then increment, then update with a user_mods dict at some future year.

I do not understand why anybody would want to do the above.

Why not simply implement the multi-year reform using Parameter class
methods and then pass the Parameters object to the Calculator class?

That is the idiom demonstrated in the test_multi_year_reform()
function in the test_parameters.py file included in pull request #321.

[talumbau]

As I mentioned before, in the dropq package, we must create the Parameters objects for the year 2013 and then increment up to the year 2015. The user input on the web application starts for the year 2015. @MattHJensen let me know that it had to be this way for now, due to some issue with how we must start with 2013 parameter values and then increment them. The details escape me, but perhaps Matt can comment on this issue.

[talumbau]

Why not simply implement the multi-year reform using Parameter class
methods and then pass the Parameters object to the Calculator class?

That is the idiom demonstrated in the test_multi_year_reform()
function in the test_parameters.py file included in pull request #321.

We can certainly support multiple ways of specifying multi-year reforms, and in fact, it seems reasonable to discuss deprecating the current way that this is done. But, it happens to be the case that we currently don't do multi-year reforms in the way that is shown in test_multi_year_reform in #321. In particular, we make use of the style of a user_mods in the form `{N : {name: [sequence of values]} }, where the ith value (either scalar or array) is the reform value for year N + i. In addition, we support

params.update(user_mods={N : {name: [sequence of values]} })

where params.current_year >= N and therefore params.start_year >= N, as long as the end of the budget window implied in user_mods matches up with the budget window for params.

[talumbau]

Back to the issue at hand, merging this is just an assertion of the behavior we depend on right now. Even if it is behavior that will go away, we can use this test (or its removal) to be explicit about that. Therefore, I would suggest merging.

[MattHJensen]

@talumbau, that makes sense to me as well. If we want this behavior to go away in the future, then we can remove the test.

[mmessick]

@talumbau, I found an issue with the test suite. When running any operation involving a line like this in this test (which appears if you try to run calc.diagnostic_table() for instance):

agi = (calc.records.c00100 * calc.records.s006).sum()

I get the error: ValueError: operands could not be broadcast together with shapes (8799,) (139651,)
This would suggest that the modifications made to the years do not correctly correlate the sizes of all the arrays in a Records class object. Just something I noticed and thought worth looking into