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

Updates to Model Fields #448

Merged
merged 3 commits into from
Jan 12, 2017
Merged

Updates to Model Fields #448

merged 3 commits into from
Jan 12, 2017

Conversation

brittainhard
Copy link
Contributor

No description provided.

@talumbau
Copy link
Member

Oh it looks like everything is an AddField for the migration. That's good. I thought certain names changed. @MattHJensen is it the case that the changes in PSLmodels/Tax-Calculator#1109 were just additions to the current_law_policy.json? Did any field change names?

@MattHJensen
Copy link
Contributor

That's right, @talumbau. Here's the relevant comment: PSLmodels/Tax-Calculator#1109 (comment)

@brittainhard
Copy link
Contributor Author

brittainhard commented Jan 11, 2017

I'l fix that quickly.
Edit: Looks like those changes have been made.

Regardless. this really needs some review.

Some of the fields I added may have problems, as I could only use other fields as guidelines -- specifically referring to how they correspond to current_law_policy.json

@talumbau
Copy link
Member

Yes, I'm looking at it closely because there are a number of changes. It actually looks good to me, except for the STD_Dep parameter. It appears to be a multi-valued parameter in current_law_policy.json, but in this PR, it's still the single value STD_Dep. I think STD_Dep should be removed and STD_Dep_0, STD_Dep_1, STD_Dep_2, STD_Dep_3, should be added along with STD_Dep_cpi as a NullBooleanField like the other CPI flags. Does that sound right @MattHJensen ?

@MattHJensen
Copy link
Contributor

@talumbau
Copy link
Member

Oops, my fault. Right, so this PR is correct with STD_Dep as just a single attribute on the model.

@brittainhard
Copy link
Contributor Author

@talumbau @MattHJensen do you think this is ready to merge?

@talumbau
Copy link
Member

I'll run this locally, including the migration to see how it goes.

@talumbau
Copy link
Member

Ran this locally. Migration looked good. Let's merge and I will push up to production tonight

@talumbau talumbau merged commit 50b6847 into master Jan 12, 2017
@brittainhard brittainhard deleted the 436_update_inputs branch February 28, 2017 18:30
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