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

Modify section headings in policy_current_law.json #2372

Merged
merged 6 commits into from
Oct 11, 2019

Conversation

Peter-Metz
Copy link
Contributor

This PR modifies certain section_1 and section_2 headings in policy_current_law.json. The need arose because there is a group of unorganized/unsectioned parameters in Tax-Brain and Tax-Cruncher because they are not assigned a section_1 (see the first screenshot for an example). In the process, I also modified a few of the section_2 headings because they were the same as the parameters' descriptions (see the second screenshot for an example).

I modified a test in test_policy.py but it still fails because the section headings do not match uguide.htmx. What is the process for updating uguide.htmx?

Un-sectioned parameters:

Screen Shot 2019-09-03 at 4 31 50 PM

Example of section_2 mirroring parameter description:

Screen Shot 2019-09-03 at 4 32 02 PM

@MattHJensen MattHJensen added the WIP label Sep 9, 2019
@MattHJensen
Copy link
Contributor

@Peter-Metz, I added a WIP label for now since the tests aren't passing. Could you ping me again when they are, and then I'll be happy to review?

@codecov
Copy link

codecov bot commented Sep 9, 2019

Codecov Report

Merging #2372 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2372   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          13      13           
  Lines        2748    2748           
======================================
  Hits         2748    2748

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3fe2304...8b07d28. Read the comment docs.

@Peter-Metz
Copy link
Contributor Author

@MattHJensen tests now pass after some modifications to uguide.htmx and test_policy.py

@MattHJensen
Copy link
Contributor

MattHJensen commented Sep 23, 2019

@Peter-Metz, thanks again for opening this PR. After reviewing this more closely, I remember some context that might be helpful:

The previous version of Tax-Brain at OSPC.org did not display parameters that were not assigned section headers. You can confirm at ospc.org/taxbrain. This is a relic of when the Tax-Calculator and Tax-Brain teams were more tightly integrated, and the Tax-Calculator team would use the absence of section headers to indicate that a parameter shouldn't appear on Tax-Brain for some reason of user safety. It was introduced in #1109.

I strongly suggest that apps using Tax-Calculator adopt the same rule for now -- i.e., don't display parameters that don't have Tax-Calculator section headers. (cc @andersonfrailey @hdoupe @jdebacker)

The Tax-Calculator project will also review how we indicate a parameter's safety for users and whether any of the un-section-headered parameters are actually safe.

@Peter-Metz, if all of this makes sense to you, then I think it would be helpful to narrow the scope of this PR to:

I also modified a few of the section_2 headings because they were the same as the parameters' descriptions (see the second screenshot for an example).

@hdoupe
Copy link
Collaborator

hdoupe commented Sep 23, 2019

I strongly suggest that apps using Tax-Calculator adopt the same rule for now -- i.e., don't display parameters that don't have Tax-Calculator section headers. (cc @andersonfrailey @hdoupe @jdebacker)

You can choose which parameters are available on compute.studio by doing something like this:

def get_inputs(meta_params_dict):
    """
    Return default parameters for Tax-Brain
    """
    metaparams = MetaParameters()
    metaparams.adjust(meta_params_dict)

    policy_params = TCParams()
    policy_params.set_state(
        year=metaparams.year.tolist(),
        data_source=metaparams.data_source.tolist(),
    )
    
    # Only keep "schema" object and parameters that have a section_1
    filtered_pol_params = OrderedDict()
    for k, v in policy_params.dump().items():
        if k == "schema" or v.get("section_1", False):
            filtered_pol_params[k] = v
    

    behavior_params = BehaviorParams()

    default_params = {
        "policy": filtered_pol_params,
        "behavior": behavior_params.dump()
    }
    meta = metaparams.dump()

    return {"meta_parameters": meta, "model_parameters": default_params}

@Peter-Metz
Copy link
Contributor Author

Thanks @MattHJensen, I'll update this PR in the next couple of days. Also thanks @hdoupe for the suggestion re: compute studio

@Peter-Metz
Copy link
Contributor Author

@MattHJensen: the latest commits reinstate the un-sectioned params (keeping the modified section_2 headings that were the same as the parameter description)

@@ -699,26 +699,18 @@ def generate_section_dictionary(html_text):
'Additional Medicare FICA': 0
},
'Social Security Taxability': {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MattHJensen sure thing. The gist is that test_policy creates a dictionary called valid_dict and compares it to policy_current_law to check section_1 and section_2 titles. For example:

valid_dict = {
       # corresponds to PCL section_1
        'Payroll Taxes': {
            # corresponds to PCL section_2
            'Social Security FICA': 0
        }

Since this PR removes some of the section_2 titles, we modify valid_dict with:

valid_dict = {
        'Payroll Taxes': {
            '': 0
        }

Does that clear things up?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, @Peter-Metz

@MattHJensen
Copy link
Contributor

Thanks for this PR, @Peter-Metz!

@MattHJensen MattHJensen merged commit 31889bd into PSLmodels:master Oct 11, 2019
@MattHJensen MattHJensen removed the WIP label Oct 11, 2019
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