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

Split policy reform and economic assumption parameters into two separate JSON files #1148

Merged
merged 8 commits into from
Jan 24, 2017
Merged

Split policy reform and economic assumption parameters into two separate JSON files #1148

merged 8 commits into from
Jan 24, 2017

Conversation

martinholmer
Copy link
Collaborator

This pull request implements in Tax-Calculator one of the key ideas in the issue #1140 conversation: the separation of policy reform parameters and economic assumption parameters into two different JSON files.

@codecov-io
Copy link

codecov-io commented Jan 18, 2017

Current coverage is 98.89% (diff: 100%)

Merging #1148 into master will increase coverage by 0.02%

@@             master      #1148   diff @@
==========================================
  Files            38         38          
  Lines          3020       3075    +55   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2986       3041    +55   
  Misses           34         34          
  Partials          0          0          

Powered by Codecov. Last update 1ba5442...4e5af19

@martinholmer martinholmer changed the title [WIP] Split policy reform and economic assumption parameters into two separate JSON files Split policy reform and economic assumption parameters into two separate JSON files Jan 19, 2017
@talumbau
Copy link
Member

This PR would seem to significantly change this interface:

www.ospc.org/taxbrain/file

The effort level is not low for this change on the web application and I am not sure I see the benefit. Is this an "example" PR or is there genuine interest in merging this to master?

@martinholmer
Copy link
Collaborator Author

@talumbau said about PR #1148:

This PR would seem to significantly change this interface:

www.ospc.org/taxbrain/file

The effort level is not low for this change on the web application and I am not sure I see the benefit. Is this an "example" PR or is there genuine interest in merging this to master?

As #1148 mentions, the rationale for making this change in Tax-Calculator, and the additional necessary changes to the TaxBrain file upload page, have been discussed at length in issue #1140. So, yes, "there is genuine interest in merging this to master".

@MattHJensen
Copy link
Contributor

MattHJensen commented Jan 20, 2017

@talumbau, there is a lot of material in #1140. Here are the relevant comments for this PR.

#1140 (comment)
#1140 (comment)
#1140 (comment)

Edit: first link was incorrect.

@@ -33,10 +33,24 @@
"behavior": {
"_BE_sub": {"2016": [0.25]}
},
"consumption": {
Copy link
Member

Choose a reason for hiding this comment

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

Is it correct that REFORM_CONTENTS should have behavior, consumption, and growth? Or should that be only in ASSUMP_CONTENTS?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, should be in ASSUMP_CONTENTS. Thanks for finding this problem, which I've fixed in commit 4e5af19.

@MattHJensen
Copy link
Contributor

This looks great, @martinholmer.

@martinholmer
Copy link
Collaborator Author

Both @MattHJensen and @talumbau have commented on pull request #1148.

Unless anybody expresses concerns, #1148 will be merged into the master branch at the end of the work day on Tuesday, January 24th.

@feenberg @Amy-Xu @andersonfrailey @GoFroggyRun @zrisher @codykallen

@MattHJensen
Copy link
Contributor

@martinholmer, do you mind merging this early today instead of late today? I'm hoping to refactor #1135 today.

@martinholmer
Copy link
Collaborator Author

@MattHJensen said:

Do you mind merging this early today instead of late today? I'm hoping to refactor #1135 today.

Merging #1148 now.

@martinholmer martinholmer merged commit b085c35 into PSLmodels:master Jan 24, 2017
@MattHJensen
Copy link
Contributor

Thanks!

@martinholmer martinholmer deleted the split-reform-assump branch January 24, 2017 16:45
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.

4 participants