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

Add GUI-generated JSON reform file to static output page #901

Merged
merged 12 commits into from
Jun 18, 2018

Conversation

lucassz
Copy link
Contributor

@lucassz lucassz commented Jun 14, 2018

Implementing #859. I kept the layout as it was, though there could be a case for placing the generated reform file below the result tables rather than above them.

@hdoupe
Copy link
Collaborator

hdoupe commented Jun 14, 2018

Hmmm it looks like there is an issue where the current Tax-Calculator version is being compared to version '0.13.0':

screen shot 2018-06-14 at 10 00 53 am

This is unrelated to the changes in this PR and in PR #902. I'm going to look into this.

@hdoupe
Copy link
Collaborator

hdoupe commented Jun 14, 2018

@lucassz this looks good. Can you format the JSON output with tabs, new lines, etc? json.dumps would work.

@lucassz
Copy link
Contributor Author

lucassz commented Jun 14, 2018

@hdoupe Done. Another idea would be to label the reform/assumption files separately.

@hdoupe
Copy link
Collaborator

hdoupe commented Jun 14, 2018

Nice, thanks. That looks much better.

Another idea would be to label the reform/assumption files separately.

Good idea. "Reform" and "Assumptions" should work.

@hdoupe
Copy link
Collaborator

hdoupe commented Jun 14, 2018

@lucassz Can you post a screen shot of how things would look once you add the labels?

@lucassz
Copy link
Contributor Author

lucassz commented Jun 14, 2018

Updated, and here are the screenshots.

screen shot 2018-06-14 at 2 30 18 pm
screen shot 2018-06-14 at 2 30 13 pm

@hdoupe
Copy link
Collaborator

hdoupe commented Jun 14, 2018

@lucassz The screenshot looks good. @Abraham-Leventhal, @andersonfrailey Do you have any thoughts on this layout?

@lucassz
Copy link
Contributor Author

lucassz commented Jun 14, 2018

For UI reasons, maybe it would be more elegant to have a collapsible box to contain the code so it's not immediately visible to nontechnical users.

@Abraham-Leventhal
Copy link

@hdoupe
I am in agreement with @lucassz on this point. I think a collapsible box or link that opens the code in a new window would be an improvement.

@hdoupe
Copy link
Collaborator

hdoupe commented Jun 14, 2018

@lucassz There are a couple stages of backwards compatibility that we could try to cover:

  • data before 1.4.0
  • data between 1.4.0 and 1.6.1

Starting in 1.4.0 the raw input data is stored in the model attribute raw_input_fields and the input data after it was parsed into the Python types (like int, float, bool) is stored in the model attribute input_fields. The JSON reform can be created from the input_fields attribute via param_formatters.to_json_reform(start_year, input_fields). So, you could do something like:

elif model.input_fields is not None:
    reform_file_contents = param_formatters.to_json_reform(start_year, model.input_fields)
    assump_file_contents = '' # assumptions were not specified because this is from the static GUI page

Don't worry about the data before 1.4.0. Backwards compatibility for the outputs was broken in 1.5.0.

Does that make sense?

@lucassz
Copy link
Contributor Author

lucassz commented Jun 14, 2018

@hdoupe Yes, thank you, that does make sense! I implemented your suggestion pretty much verbatim and it works well.

@hdoupe
Copy link
Collaborator

hdoupe commented Jun 14, 2018

Nice, this is looking sharp. One more thing: can you apply this to the behavioral outputs page? The dynamic behavioral sim uses the same taxbrain/results.html page. You just have to update dyanamic.views.behavior_results.

You can find the TaxSaveInputs model class from the static run at the micro_sim attribute on the DynamicBehaviorSaveInputs model class.

@lucassz
Copy link
Contributor Author

lucassz commented Jun 14, 2018

@hdoupe Should that display the (few) additional parameters for the dynamic configuration, or just the ones for the initial microsimulation?

@hdoupe
Copy link
Collaborator

hdoupe commented Jun 14, 2018

@lucassz That should show the parameters from the initial simulation and from the dynamic simulation.

@lucassz
Copy link
Contributor Author

lucassz commented Jun 14, 2018

The new commit implements that and also gets rid of a bunch of hardcoding of the OSPC hostname. Is there any particular reason it was in there?

@hdoupe
Copy link
Collaborator

hdoupe commented Jun 14, 2018

Alright, looks good. Nope it was there when I started working on this project. Thanks for cleaning that up.

I think this is ready to be merged once you fix the merge conflicts.

micro = model.micro_sim.unique_inputs
micro = (model.micro_sim.unique_inputs if model.micro_sim
else TaxSaveInputs())

if (micro.json_text is not None and (micro.json_text.raw_reform_text or
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lucassz The bug is in how the test was written. While this works, I think it makes more sense to fix the bug in the test. What you could to fix this is create a OutputUrl model instance for the microsimulation run in the test and add it as the micro_sim attribute on the DynamicOutputUrl instance.

You can use the get_taxbrain_model helper function to create the url instance. One example for how to use it for the micro-sim model is here: https://github.com/OpenSourcePolicyCenter/PolicyBrain/blob/master/webapp/apps/taxbrain/tests/test_views.py#L756-L758.

Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thank you, looking into this now.

model.deprecated_fields = None
model.raw_input_fields = {}
model.input_fields = {}
model.deprecated_fields = {}
model.tax_result = "unrenderable"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove the code that changes raw_input_fields, input_fields, and deprecated_fields. There's a similar test in taxbrain/tests/test_views that has these lines of code. You can remove those, too.

@@ -788,6 +784,26 @@ def behavior_results(request, pk):
'webapp_version': webapp_vers_disp}

model = url.unique_inputs

first_year = model.first_year
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some cases from really old runs where first_year is None. This can be fixed with first_year = model.first_year or START_YEAR, defaulting to the current start year if one is not specified.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If first_year is None, then that will break the to_json_reform call.

model.tax_result = "unrenderable"
if start_year_is_none:
model.first_year = None
model.micro_sim = get_taxbrain_model(fields,
taxcalc_vers="0.14.2",
webapp_vers="1.3.0")
model.save()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use the microsim fields here. Otherwise the form validation will fail because _BE_inc will be included.

Something like this could work:

        micro_sim_fields = get_post_data(start_year, _ID_BenefitSurtax_Switches=False)
        micro_sim_fields['first_year'] = start_year
        micro_sim = get_taxbrain_model(micro_sim_fields,
                                       taxcalc_vers="0.14.2",
                                       webapp_vers="1.4.0")

@@ -788,6 +784,26 @@ def behavior_results(request, pk):
'webapp_version': webapp_vers_disp}

model = url.unique_inputs

first_year = model.first_year or START_YEAR
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to be int(START_YEAR) since START_YEAR is a string.

@hdoupe
Copy link
Collaborator

hdoupe commented Jun 18, 2018

@lucassz This looks good to me. Thanks for implementing this feature. This will be very helpful in allowing our users to see how their inputs are transformed and submitted to Tax-Calculator. This will help us find more bugs and make the file upload capabilities more accessible.

@Abraham-Leventhal @lucassz I like the idea of moving the JSON into something like a collapsible box. The outputs page is about to be re-designed. I encourage you to advocate for a feature like this once we begin the re-design and implementation discussions.

@hdoupe hdoupe merged commit 07b4af2 into ospc-org:master Jun 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants