Make dropq test fail like in issue 1367; then fix bug #1387
Conversation
|
Commit 7016899 makes the dropq_utils.py dropq_calculate function better able to handle policy reforms that are focused on payroll taxes rather than on income taxes. But these changes do not fix the bug detected by the revised dropq test implemented in commit 6610a62. The Travis-CI error associated with 6610a62 seems to be a GitHub error: the failure was only on Python 3.5 because the tests never statted. All the tests passed under Python 2.7 and 3.6. |
Codecov Report
@@ Coverage Diff @@
## master #1387 +/- ##
==========================================
+ Coverage 99.64% 99.64% +<.01%
==========================================
Files 38 38
Lines 2819 2826 +7
==========================================
+ Hits 2809 2816 +7
Misses 10 10
Continue to review full report at Codecov.
|
|
Commit 5a79fa4 fixes the bug. This bug has existed for years, back to the beginning of work on utils.py functions. For example, look at the add_weighted_income_bins function as it existed on February 4, 2016, by following this link. So, this bug has nothing to do with the recent refactoring of the dropq code or with the changes in pandas when moving from version 0.18 to 0.20. It has appeared now simply because @brittainhard happened to specify a very particular policy reform that revealed the bug. |
|
More about the bug in the The revisions to the taxcalc/utils.py Both problems would cause the The first problem was caused by the less-than-perfect precision of floating-point arithmetic performed by computers. The old code counted on perfect precision to get the top edge of the last bin to be precisely equal to the maximum value of the continuous variable. But the less-than-perfect precision of the computer caused a few filing units to not be placed into the last bin. You can see that is the case because commenting out just one of the new lines of code (see below) causes the error to reoccur. The following code works fine: while the following code generates a fatal error in the new test: The second problem was that the old So, the @MattHJensen @feenberg @Amy-Xu @andersonfrailey @GoFroggyRun @codykallen |
|
Note that the Saturday afternoon test failures are not really Tax-Calculator test failures, but the inability of Continuum Analytics web servers to transfer conda packages to Travis-CI. I don't know which end of that transaction is not working. |
|
Pull request #1387 changes the basic dropq algorithm in two ways. I want to emphasize this here so that those involved in the original development of that algorithm can give these changes a careful review. Here is the relevant part of the I added the NEW LINE so that payroll taxes would change as well as income taxes. I REVISED the other line so that the mask is defined by changes in the combined tax, not just by changes in the income tax amount. Does this make sense? Or has my lack of knowledge about the dropq algorithm led me to make a mistake? |
|
I don't know the meaning of the |
|
I would prefer it if we left the dropq code dependent on the income tax
only. The reason is that we want to drop a few records that affect the
tables in every table row. If we drop a record that does not contribute to
the row, not much confidentiality is obtained. By considering FICA in the
dropq selection we include records with no positive or negative tax to be
among those dropped. All they would need is a small amount of earnings but
no other cells would be affected. The dropq procedure depends on the
dropping of records having a (hopefully small but not zero) effect on the
results.
In any case, it isn't important that every record be eligible for
dropping.
dan
…On Sat, 3 Jun 2017, Martin Holmer wrote:
Pull request #1387 changes the basic dropq algorithm in two ways. I want to
emphasize this here so that those involved in the original development of
that algorithm can give these changes a careful review.
Here is the relevant part of the dropq_calculate function with the two
new/revised lines marked:
# optionally compute mask
if mask_computed:
# create pre-reform Calculator instance with extra income
recs1p = Records(data=taxrec_df.copy(deep=True),
gfactors=growfactors_pre)
# add one dollar to total wages and salaries of each filing unit
recs1p.e00200 += 1.0 # pylint: disable=no-member
recs1p.e00200p += 1.0 # pylint: disable=no-member <---- NEW L
INE ----
policy1p = Policy(gfactors=growfactors_pre)
# create Calculator with recs1p and calculate for start_year
calc1p = Calculator(policy=policy1p, records=recs1p,
consumption=consump)
while calc1p.current_year < start_year:
calc1p.increment_year()
calc1p.calc_all()
assert calc1p.current_year == start_year
# compute mask that shows which of the calc1 and calc1p results diff
er
res1 = results(calc1)
res1p = results(calc1p)
mask = (res1.combined != res1p.combined) <---- REVI
SED LINE ----
else:
mask = None
I added the NEW LINE so that payroll taxes would change as well as income
taxes.
Apparently this dropq code was written before there was an appreciation that
FICA is an individual tax, not an income-tax filing-unit tax.
I REVISED the other line so that the mask is defined by changes in the
combined tax, not just by changes in the income tax amount.
Does this make sense? Or has my lack of knowledge about the dropq algorithm
led me to make a mistake?
@MattHJensen @feenberg @talumbau
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the
thread.[AHvQVWvxRwQ7zbRfLcu2TWseq73IcHnRks5sAdB_gaJpZM4Nu8UV.gif]
|
|
@feenberg said:
OK, I've changed back to the original definition of |
martinholmer
changed the title
martinholmer
changed the title
|
@martinholmer, thanks for identifying and resolving this problem. I agree w Dan about using iitax only for the mask. The other changes look good to me too. Would be helpful to have @Amy-Xu's review as well. |
|
@MattHJensen said:
The definition of the |
| year_n = analysis_year - start_year | ||
| reform = { | ||
| '_FICA_ss_trt': [0.2] | ||
| } |
There was a problem hiding this comment.
Just to confirm: the reason of switching to this reform is that it will change not only iitax but also payroll right?
There was a problem hiding this comment.
Martin got started on this PR because that reform failed: #1387 (comment)
There was a problem hiding this comment.
@Amy-Xu, @MattHJensen is exactly correct. The new reform (with just a payroll tax rate increase) was the reform reported by @PeterDSteinberg as causing a fatal error. So, I revised the test to be like in his bug report and worked until I found what was causing that test failure. Does this make sense?
|
@martinholmer said
All the changes in this PR look good to me but I guess I'm confused on two things. First is regarding the first problem -- if this line of code is a problem, why is it only a problem for payroll reforms but not iitax reforms? Shouldn't it always result in missing the last record? Second is about what records need to be included in bins. I believe we have ran into the second problem before that negative expanded income people are not included in tables, which was discussed in this issue when Joe Sullivan brought it up. After we checked the TPC table example linked in that issue, we concluded it was fine to leave out negative income people. Do we need to refer to TPC tables or we simply want to make everything look consistent? @MattHJensen |
|
@Amy-Xu asked first:
I think this a floating-point arithmetic precision issue. I don't think it has anything to do with payroll tax vs income tax reforms. My guess is that the bug report happened to involve a FICA-only reform, but this problem could have arisen with any kink of tax reform. @Amy-Xu also asked:
The second logic problem in #1387 has nothing to do with the issue of whether or not TaxBrain tables show people with negative expanded_income. It has to do with avoiding a fatal error in the calculations. I don't think the Does all this make sense? |
|
@martinholmer You're right this function hasn't been called by TaxBrain with |
|
@Amy-Xu said:
Yes and Yes. There is no change in the basic logic of the |
taxcalc/utils.py
Outdated
| cumsum_range = max_cumsum - min_cumsum | ||
| bin_width = cumsum_range / float(num_bins) | ||
| bin_edges = [-9e99] + list(np.arange(1, (num_bins + 1)) * bin_width) | ||
| bin_edges[-1] = 9e99 # raise top of last bin to include all observations |
There was a problem hiding this comment.
Hmmmm there's still a minor issue with weighted_by_income_measure option set to True. Since bin_width is stack on top of zero due to the logic of list(np.arange(1, (num_bins + 1)) * bin_width) snippet, the first bin would be larger than expected when cumulated sum is weighed by income_measure and push all other bin edges to the right by the amount equal to the absolute value of min_cumsum, doesn't it?
There was a problem hiding this comment.
@Amy-Xu said:
Ignore the previous comment. This section looks good to me.
There was a problem hiding this comment.
That comment was a previous one. Sorry about that. Do you think this current comment is valid?
|
@martinholmer Could you take a look at my latest comment above? Sorry about back and forth on this -- just want to make sure the bins with |
|
@Amy-Xu, You are commenting at two different levels in GitHub, so I have lost track about what you mean by "my latest comment above". |
|
@martinholmer my bad, this one: |
|
@Amy-Xu said about this code:
Let's review the pandas.cut function. Part of the documentation says this: If this explanation is not an effective response to your concerns, then I'm not understanding exactly what your concern is. The code is exactly the same as before except that the bottom of the first bin has been made very small and the top of the last bin has been made very large. If I'm not being responsive, my apologies. Ask the question in a different way and maybe I'll understand better your concerns. |
I agree with this part when I would assume when bin_width is calculated as (max_cumsum - min_cumsum)/num_bins, it suggests that people get cumsum_temp between -40 to -20 are thrown into the lowest bin. If that's the expectation, that doesn't seem consistent with reality. As the bin_edges looks like [-9e99, 20, 40, ..., 140, 9e99], the lowest would include people from -40 to 20 instead of -40 to -20. Similarly the bins afterward would be pushed right by about 40. Is this a clearer way to describe my concern? If this is intended, I have finished review. This PR looks good to me. |
|
@Amy-Xu said:
Thanks for being patient with me in expressing your concerns. I think you are right about this, so I'll change it. I'd appreciate your review of the change I make later this afternoon. |
|
@martinholmer Thank you very much for the modification. I really appreciate your time on this. My point was a minor issue. Since no one has risen any problem with setting up bins in the current form, I'm happy not to worry anything about it. This PR looks good to me. |
Commit 6610a62 in this pull request revises an existing dropq test to have a policy reform like the one supplied by @PeterDSteinberg on June 2, 2017, in issue #1367. The revised test generates a fatal error indicating a bug somewhere in the Tax-Calculator code base. A subsequent commit in this pull request will fix this bug.
The policy reform in the new failing test consists of a single reform provision: an increase in 2017 of the FICA tax rate from 0.124 to 0.200. But other values of the FICA tax rate cause the exact same test to pass as shown below:
For the record, here is the error traceback report: