-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add reverse operator #790
Add reverse operator #790
Conversation
@hdoupe, I got a value error running the example you gave. Also noticed that we should add a user tip before the "Read the Docs" Bullet. Something like, "You may specify a parameter change in the year before the Start Year with the |
Thanks for the bug report. The most recent commit resolves the issue. That error should only affect the edit page. |
Good idea. Yes, I agree. I'll add something along those lines. |
Changes up to |
@hdoupe, I would expect to see a change in results in 2017 with this reform: http://ospc-taxes7.herokuapp.com/taxbrain/1494/. |
^^ Interestingly, the reverse operator does appear to be working in other contexts based on the differences in 2017 between http://ospc-taxes7.herokuapp.com/taxbrain/1498/ and http://ospc-taxes7.herokuapp.com/taxbrain/1499/. |
@MattHJensen Ok, I submitted the parameters from your reform (+ set FICA_ss_trt to 0.78 so I could identify the run) and dug out the keywords to dropq:
It looks like everything is passed to Tax-Calculator correctly. Have you checked the results using Tax-Calculator directly? |
Here are the input and output from doing run using the edit parameters page created fromhttp://ospc-taxes7.herokuapp.com/taxbrain/1494/:
http://ospc-taxes7.herokuapp.com/taxbrain/1501/ They look the same. Maybe the magnitude change is just really small? |
Ok, the right way to test would be to just run the reform through tc CLI and see if the 2017 change is small, but I'm not in a position to do so at the moment. |
Ok, good idea. Here are the results:
Looks good to me. |
@hdoupe, @martinholmer, am I right to think that this ^^ might indicate a bug in Tax-Calculator? I would have thought that the weighted difference table for 2017 should show a positive value in the AllTax column (and some others) because of the "_cpi_offset": {"2016": [-0.005]}, |
@hdoupe, it does seem like this PR has PolicyBrain accurately reflecting the results from Tax-Calculator, even if those results don't currently make sense to me. |
@MattHJensen asked
I'm not sure. I ran another reform where I only changed the cpi_offset parameter, and there isn't any change in revenue in 2017. From 2018 to 2019, the difference between the changes is about $2 billion followed by a $2.6 change from 2019 to 2020. This pattern carries on until 2027. So, perhaps, the change in revenue is negligible at the beginning. Then, the changes grow over time and become more noticeable. |
@MattHJensen said:
Ok, thanks for the thorough review. |
@MattHJensen and @hdoupe, the following results should help clear up the mystery in #790. Here is a simple Python script:
If we execute that script, we get these results:
So, why isn't the personal exemption amount under the reform (policy2) lower than under current-law (policy1) beginning in 2016? The answer is because we have known values of |
The operator looks good to me +1. Thanks for adding this enhancement @hdoupe. |
@martinholmer, thanks very much for the Tax-Calculator illumination. @hdoupe, this looks good to me. |
Thanks for the review @MattHJensen, @martinholmer, and @GoFroggyRun |
This PR adds the reverse operator discussed in issue #763.
Basic rules as described in this comment in #763 :