Skip to content
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

Error using problematic r1.json file when doing a macro reform simulation #612

Closed
brittainhard opened this issue Aug 8, 2017 · 23 comments
Closed
Assignees
Labels
Milestone

Comments

@brittainhard
Copy link
Contributor

brittainhard commented Aug 8, 2017

Probably similar to the bug in #578

@brittainhard brittainhard changed the title Issue when Using Problematic r1.json File when doing a Macro Reform Error using problematic r1.json file when doing a macro reform simulation Aug 8, 2017
@brittainhard brittainhard self-assigned this Aug 8, 2017
@martinholmer
Copy link
Contributor

martinholmer commented Aug 11, 2017

@brittainhard, What is "problematic" about the r1.json reform file?

I thought your recent fixes with respect to start year on the file-upload page had fixed the bug reported in issue #578.

What error did you get? Was it a Tax-Calculator error or a TaxBrain error?

@brittainhard
Copy link
Contributor Author

brittainhard commented Aug 14, 2017

@martinholmer the feature that allowed file uploads to be used for simulations came with the earlier PR to enhance file-input interactions on webapp.

The view that handles the macro simulations does support file data, but does not support the r1.json file that was causing the issues in #578. Doesn't handle the start_year edge case that was causing the problem in #578.

EDIT: the error was in taxbrain.

@brittainhard
Copy link
Contributor Author

To further clarify, this is not a bug with the initial file upload, only when you use the file upload to run simulations. Really comes down to an input error.

@martinholmer
Copy link
Contributor

@brittainhard said about issue #612:

To further clarify, this is not a bug with the initial file upload, only when you use the file upload to run simulations. Really comes down to an input error.

I don't understand what kind of error the r1.json reform generates.
The reform looks pretty straight-forward to me (see below).
What error message do you get from Tax-Calculator?

Here are the r1.json file contents:

// Assume reform with the following provisions:
// - adhoc raises in OASDI maximum taxable earnings in 2018, 2019 and 2020,
//     with _SS_Earnings_c wage indexed in subsequent years
// - raise personal exemption amount _II_em in 2018, keep it unchanged for
//     two years and then resume its price indexing in subsequent years
// - implement a 20% investment income AGI exclusion beginning in 2019
{
    "policy": {
        "_SS_Earnings_c": {"2018": [400000],
                           "2019": [500000],
                           "2020": [600000]},
        "_II_em": {"2018": [8000]},
        "_II_em_cpi": {"2018": false,
                       "2020": true},
        "_ALD_InvInc_ec_rt": {"2019": [0.20]}
    }
}

@brittainhard
Copy link
Contributor Author

@martinholmer the error is this:

Traceback (most recent call last):
  File "/Users/bhard/anaconda/envs/aei_dropq/lib/python2.7/site-packages/flask/app.py", line 1997, in __call__
    return self.wsgi_app(environ, start_response)
  File "/Users/bhard/anaconda/envs/aei_dropq/lib/python2.7/site-packages/flask/app.py", line 1985, in wsgi_app
    response = self.handle_exception(e)
  File "/Users/bhard/anaconda/envs/aei_dropq/lib/python2.7/site-packages/flask/app.py", line 1540, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "/Users/bhard/anaconda/envs/aei_dropq/lib/python2.7/site-packages/flask/app.py", line 1982, in wsgi_app
    response = self.full_dispatch_request()
  File "/Users/bhard/anaconda/envs/aei_dropq/lib/python2.7/site-packages/flask/app.py", line 1614, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/Users/bhard/anaconda/envs/aei_dropq/lib/python2.7/site-packages/flask/app.py", line 1517, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/Users/bhard/anaconda/envs/aei_dropq/lib/python2.7/site-packages/flask/app.py", line 1612, in full_dispatch_request
    rv = self.dispatch_request()
  File "/Users/bhard/anaconda/envs/aei_dropq/lib/python2.7/site-packages/flask/app.py", line 1598, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/Users/bhard/Documents/webapp-public/deploy/taxbrain_server/flask_server.py", line 123, in elastic_endpoint
    elast_params = float(elast_params[0])

Not the same issue, but definitely coming from the same file. This feature wasn't designed with that reform file in mind, so we have to deal with this edge case.

My hunch is that the problem is similar to the #578 problem. Haven't made work on it yet, but the fix will be added to the next release.

@martinholmer
Copy link
Contributor

@brittainhard posted in TaxBrain issue #612 the following as the last item in the error traceback:

File "/Users/bhard/Documents/webapp-public/deploy/taxbrain_server/flask_server.py", line 123, in elastic_endpoint
   elast_params = float(elast_params[0])

This error message shows that the error occurred in TaxBrain (before Tax-Calculator was ever called) in the elastic_endpoint() at the statement marked below with # <---------------.

@app.route("/elastic_gdp_start_job", methods=['POST'])
def elastic_endpoint():
    # TODO: this assumes a single year is the key at highest
    # level.
    user_mods = tuple(json.loads(request.form['user_mods']).values())[0]
    year_n = int(request.form['year'])
    print('user_mods', user_mods, 'year_n', year_n)
    user_mods = json.loads(request.form['user_mods'])
    elast_params = None if 'elasticity_params' not in request.form else json.loads(request.form['elasticity_params'])
    first_budget_year = None if 'first_budget_year' not in request.form else request.form['first_budget_year']
    user_mods = {int(k): v for k, v in user_mods.iteritems()}
    elast_params = elast_params or user_mods.values()[0].pop('elastic_gdp', elast_params)
    if not isinstance(elast_params, (float, int)):
        elast_params = float(elast_params[0])  # <----------------------------------------
    print("elast params", elast_params, " user_mods: ", user_mods)

Notice that this error has nothing to do the first budget year, but rather has to do with the macroeconomic GDP elasticity passed to the elastic_endpoint.

@brittainhard, Can you show us the value of elast_params just before the error?
What is elast_params[0]? It either doesn't exist or cannot be converted to a float.
How did it get to be this way? You didn't show us the results of the print statement
that showed the contents of user_mods. Can you provide us all this requested information?

@MattHJensen @hdoupe

@brittainhard
Copy link
Contributor Author

brittainhard commented Aug 14, 2017

@martinholmer thanks for looking into this for me. Your insight is definitely helpful.

This is on deck for the next release, but it is not currently the highest priority. My focus right now is on getting @hdoupe access to do his own deployments. After that is ready, I can begin to tackle this problem, and other issues that need fixing before the next release. I can then provide you with the debugging information.

@martinholmer
Copy link
Contributor

@brittainhard said:

This (issue #612) is on deck for the next release, but it is not currently the highest priority.
My focus right now is on getting @hdoupe access to do his own deployments.

OK, good. So, where do you stand in working with @hdoupe to get TaxBrain 1.0.1 operational?

@brittainhard
Copy link
Contributor Author

@martinholmer 1.0.1 is up on production right now. We are going to fix the bugs that @hdoupe found and add them to the 1.0.2 release.

@brittainhard brittainhard added this to the 1.0.2 Release milestone Aug 15, 2017
@martinholmer
Copy link
Contributor

@brittainhard said:

[TaxBrain version] 1.0.1 is up on production right now.
We are going to fix the bugs that @hdoupe found and add them to the 1.0.2 release.

I don't understand what you mean by "is up on production right now".
When I go to TaxBrain is still says it's the old version:

screen shot 2017-08-15 at 12 14 32 pm

I thought the new version was supposed to drop the hash and say this:

TaxBrain 1.0.1  Tax-Calculator 0.9.1

Am I confused?

@hdoupe

@brittainhard
Copy link
Contributor Author

@martinholmer where on the app are you seeing that text in that picture you posted?

I wasn't aware that we were supposed to show TaxBrain 1.0.1 Tax-Calculator 0.9.1. Matt wanted 1.0.1 to use taxcalc 0.9.0. We might upgrade it for the next release, so it looks like the taxcalc version is correct.

I was also not aware that we were supposed to show the taxcalc version and the taxbrain version. Can you clarify a bit?

@martinholmer
Copy link
Contributor

@brittainhard asked:

where on the app are you seeing that text in that picture you posted?

Go to URL http://www.ospc.org/taxbrain/
and click on What is TaxBrain?, which is just below the Start Exploring button
and then scroll to the bottom of the page that is revealed.

This is the only place that TaxBrain says what version it is, so it should be up-to-date and informative. We discussed this months ago, so maybe you've forgotten. Here is a summary of the prior conversation:

The commit hash is not useful (I couldn't find the one listed now) and is "scary" for non-developers. And the current version of the info says nothing about what version of TaxBrain is running. So, we agreed to have something like this:

                    Code Versions
TaxBrain 1.0.1  Tax-Calculator 0.9.0  [etc. eg: OG-USA x.x.x]

There is no need for any commit numbers because each repo has the commit for each release/version. (Sorry I was confused about what T-C version Matt wanted; I see now that he wanted to pair TB 1.0.1 with T-C 0.9.0.)

If we don't have this kind of information on the What is TaxBrain? page, nobody knows what version of TaxBrain they're using.

Can we make this change immediately?

@MattHJensen @hdoupe

@martinholmer
Copy link
Contributor

martinholmer commented Aug 15, 2017

@martinholmer said in issue #612:

This [at the bottom of the What is TaxBrain? page] is the only place that TaxBrain says what version it is, so it should be up-to-date and informative.

Actually, this is not correct because the top of the Static Results page shows this same information:

screen shot 2017-08-15 at 1 24 52 pm

So, we need to eliminate the commit hash here as well. And the version of TaxBrain being used it not mentioned (otherwise there would be some mention of 1.0.1).

Also, what does [ID: 10045] mean? Is that really important information? If so, what does it mean? Without any explanation it is simply confusing to TaxBrain users.

And a final suggestion. While it is useful to show the time of day the results were generated, not specifying the time zone is highly confusing. If the clocks on the servers are all set to UTC (or as they said in the English Empire, GMT), then we should say so.

@GoFroggyRun and @hdoupe, Do either of you know how to change the TaxBrain code along the lines of the discussion at the end of issue #612? If so, it would be great if you could prepare a pull request that clarifies what the Code Versions are. Thanks in advance.

@hdoupe
Copy link
Collaborator

hdoupe commented Aug 15, 2017

@martinholmer I'll look into this unless @GoFroggyRun already has an idea of how to fix this.

@martinholmer
Copy link
Contributor

@hdoupe said:

I'll look into this unless @GoFroggyRun already has an idea of how to fix this.

Great! Any suggestions about where Hank should start looking in the code, @GoFroggyRun?

@GoFroggyRun
Copy link
Contributor

@martinholmer, @hdoupe :

It seems to me that modifying the code line here will do the work.

@martinholmer
Copy link
Contributor

@GoFroggyRun said:

It seems to me that modifying the code line here will do the work.

Yes, that line seems to generate the line right under the Static Results title.

@hdoupe, Can you try to change it? But we still don't know what [ID: 10045] means. It's hardwired, so it seems as if it's just a mistake. Unless @brittainhard explains its importance, I think you should just drop it.

Meanwhile, @GoFroggyRun, can you change the version information at the bottom of the What is TaBrain? page along the lines discussed in #612?

Is there a way for the two of you to test your changes while each of you prepare a pull request?
If so, please post a screen shot of what the new code produces in the pull request discussion.
Thanks.

@hdoupe
Copy link
Collaborator

hdoupe commented Aug 15, 2017

@martinholmer @GoFroggyRun I think I it will be easier to change this part of the code:

tcversion_info = taxcalc._version.get_versions()

taxcalc_version = ".".join([tcversion_info['version'], tcversion_info['full'][:6]])

...
def view_function(request)
    ...
    if unique_url.taxcalc_vers != None:
        pass
    else:
        unique_url.taxcalc_vers = taxcalc_version
    ...

Then change this to something like

tcversion_info = taxcalc._version.get_versions()

taxcalc_version_long = ".".join([tcversion_info['version'], tcversion_info['full'][:6]])
taxcalc_version = tcversion_info['version']

...
def view_function(request)
    ...
    if unique_url.taxcalc_vers != None:
        pass
    else:
        unique_url.taxcalc_vers = taxcalc_version

    if unique_url.taxcalc_vers_long != None:
        pass
    else:
        unique_url.taxcalc_vers_long = taxcalc_version_long
     ...

Then, we don't have to change the html code. I think that the html code is the only place where unique_url.taxcalc_vers is used.

@martinholmer
Copy link
Contributor

@hdoupe suggested alternative approach to #612 code version changes.

OK, that makes sense. How do we get the TaxBrain version? Now it is 1.0.1 but will be different in the future.

@GoFroggyRun, Does this approach make sense to you?

@brittainhard

@hdoupe
Copy link
Collaborator

hdoupe commented Aug 15, 2017

Actually, do we need to keep track of the commit info? If not it would be nice to drop it. Otherwise, a new variable would have to be created in OutputUrl to keep track of the full version.

@brittainhard @martinholmer @GoFroggyRun

@GoFroggyRun
Copy link
Contributor

@martinholmer @hdoupe , the alternative approach makes sense to me as well.

@martinholmer said:

Meanwhile, @GoFroggyRun, can you change the version information at the bottom of the What is TaBrain? page along the lines discussed in #612?

No problem. I'll take a look at this.

@martinholmer
Copy link
Contributor

@hdoupe asked:

Actually, do we need to keep track of the commit info? If not it would be nice to drop it. Otherwise, a new variable would have to be created in OutputUrl to keep track of the full version.

No, please drop the commit number per my earlier comment:

There is no need for any commit numbers because 
each repo has the commit for each release/version.

@martinholmer
Copy link
Contributor

Whatever @brittainhard was concerned about in #612 seems to be fixed in TaxBrain 1.0.2 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants