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

Update README.md #702

Merged
merged 5 commits into from
Oct 27, 2017
Merged

Update README.md #702

merged 5 commits into from
Oct 27, 2017

Conversation

hdoupe
Copy link
Collaborator

@hdoupe hdoupe commented Oct 12, 2017

This PR updates the README.md document. I tried to give a description of PolicyBrain that had a technical tilt. I included the new local deployment steps. I still need to add the production and test server deployment steps. However, I think this would be a little too much for the README.md document. So, I think we should move the deployment information to a separate document. I also need to add a contributing guide.

I appreciate all feedback on this document. It could be formatted completely differently if we think that would be helpful. I recommend checking out the README.md documents for other projects such as Pandas and NumPy. I liked NumPy's concise style and use of bullet points. I thought that made it easier to read. It seems like there is a fairly large variation in README styles. For example, compare Scipy's README with NumPy's.

@martinholmer
Copy link
Contributor

@hdoupe, Big improvement in the README.md text. Thanks.

@MattHJensen
Copy link
Contributor

Looks great. +1

@hdoupe
Copy link
Collaborator Author

hdoupe commented Oct 16, 2017

@brittainhard In the following line, I wanted to briefly describe PolicyBrain's technology stack. Do you have any input on how this was worded?

"PolicyBrain is a Django app which is deployed on Heroku and uses Flask, Celery, and Redis to schedule jobs."

Specifically, I'm not sure if I got the "...uses Flask, Celery, and Redis to schedule jobs." part right.

@brittainhard
Copy link
Contributor

@hdoupe i'll take a look at that wording.

@hdoupe
Copy link
Collaborator Author

hdoupe commented Oct 18, 2017

Thanks @brittainhard

@brittainhard
Copy link
Contributor

@hdoupe I think you are on point with the stack description. LGTM.

However, this got me thinking. I'm not sure someone can get the local application running without taxpuf. This is an issue that is long-standing. We need a way for people to develop on the application without needing taxpuf, since it is private.

I don't know if there is an issue open for this or not. @MattHJensen @PeterDSteinberg do you recall if we came up with a solution for this?

@hdoupe
Copy link
Collaborator Author

hdoupe commented Oct 18, 2017

@brittainhard Thanks!

However, this got me thinking. I'm not sure someone can get the local application running without taxpuf. This is an issue that is long-standing. We need a way for people to develop on the application without needing taxpuf, since it is private.

I ran into this problem this afternoon. I re-cloned my local PolicyBrain directory after the release and forgot to include the PUF file until this afternoon. I was able to look at the parameter input pages, but I got an error when I tried to submit a reform. By the end of the year, we will have an option for users to use the CPS. At that point, you could run the full model without the PUF file.

@martinholmer
Copy link
Contributor

martinholmer commented Oct 19, 2017

@brittainhard said in the discussion of PR #702:

However, this got me thinking. I'm not sure someone can get the local application running without taxpuf. This is an issue that is long-standing. We need a way for people to develop on the application without needing taxpuf, since it is private.

This problem will be solved once PolicyBrain starts using the taxcalc>=0.12.0 package. One of the enhancements in 0.12.0 is to make the taxbrain interface or tbi (nee dropq) functions work with either the puf.csv or cps.csv input files. See Tax-Calculator pull request 1577 for details.

@MattHJensen @hdoupe @GoFroggyRun

@hdoupe
Copy link
Collaborator Author

hdoupe commented Oct 23, 2017

Unless someone objects, I plan to merge this on Wednesday (10/25).

@hdoupe hdoupe merged commit f566282 into ospc-org:master Oct 27, 2017
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