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

examples refactor to user notebooks connects with #129 #135

Merged
merged 5 commits into from
Feb 28, 2017
Merged

examples refactor to user notebooks connects with #129 #135

merged 5 commits into from
Feb 28, 2017

Conversation

bbengfort
Copy link
Member

@bbengfort bbengfort commented Feb 23, 2017

One of the pieces of advice I received at PyCon was to encourage new contributors to create Pull Requests as soon as possible so that the project can be discussed. This PR is an experiment to see how that could work.

This PR addresses Issue #134 and Issue #129 -- though the user study is just a stub, and is not yet complete.

@bbengfort bbengfort added the type: task non-code related task label Feb 23, 2017
@bbengfort bbengfort added this to the Version 0.3.4 milestone Feb 23, 2017
@bbengfort bbengfort added the review PR is open label Feb 23, 2017
@bbengfort
Copy link
Member Author

Ok, I'm already glad I did this. It looks like I can add more commits to this PR simply by pushing commits to my develop branch. That is really good info, and it really does mean that the PR should be the first thing a contributor does.

@NealHumphrey
Copy link
Contributor

@bbengfort I personally would encourage people working on forks to create the pull requests from their feature branch instead of making the pull request from their develop branch. That way if the pull request is rejected or needs some back-and-forth, they can still merge the latest changes into their forked develop branch without needing to know how to rebase and without muddying it with their local changes.

A pull request made from a feature branch works the same way with being able to add more commits to the PR and they'll update here.

@@ -0,0 +1,64 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

To continue the demo of how pull requests from new users might go:

Looks like a good start at a notebook!

Copy link
Member Author

Choose a reason for hiding this comment

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

Excellent, thanks for going through this process with me! So in this case, I'm seeing "change requested" - so how do I push something back to you to accept the "change"?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've usually just had a conversation on this line item - so you might respond 'Fixed in most recent commit'. I have an entry in this feed that shows your commit "text frequency visualizer" and I can look at just the changes in that commit, or I can look at the whole net effect of the pull request.

If this branch was protected to require reviews, there would be a button I could click at the bottom of this page to approve the review, if I remember correctly. All this review stuff is just like 3 months old so I'm still getting used to it.

@coveralls
Copy link

coveralls commented Feb 23, 2017

Coverage Status

Coverage remained the same at 69.348% when pulling c4dd164 on bbengfort:develop into 98829fb on DistrictDataLabs:develop.

"\n",
"- 1,200,378 paragraphs (17.639 mean paragraphs per file)\n",
"- 2,058,635 sentences (1.715 mean sentences per paragraph).\n",
"\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

I can add multiple comments to a single review:

Would it be worth calculating these in the notebook rather than just typing them?

Copy link
Member Author

Choose a reason for hiding this comment

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

It may be worth it; I was just adding the stub, you can see that I went with more simple data loading rather than using a CorpusReader object, which is what spit out the description you saw above. I have some text frequency visualizers in there now, which also show similar information, but with yellowbrick!

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense! Mostly I was just trying to think of something to comment on for the demo :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Totally get it - no worries; just trying to do the review thing!

Copy link
Contributor

@NealHumphrey NealHumphrey left a comment

Choose a reason for hiding this comment

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

I added two line-item comments. In this case, I'll demo the 'request changes' review type. This is the comment that I enter when clicking 'Submit review'

@NealHumphrey
Copy link
Contributor

To comment on the original premise from your PyCon recommendation - definitely agree, people tend to put off Pull requests until they think the feature is done, but it's a great way to have discussion on code approaches and specifics. Good stuff!

@bbengfort
Copy link
Member Author

@NealHumphrey awesome, thank you for going through this process! I did think about doing a feature branch, but I figured that most contributors (e.g. not us) would probably actually just modify master and submit a PR to develop. I was just curious to see what that would look like when we merged.

I'm still in progress, not ready to close or merge, but I've added another commit.

@rebeccabilbro - remind me to discuss the auto-labels in Waffle with you.

@bbengfort bbengfort added in progress label for Waffle board and removed review PR is open labels Feb 23, 2017
@bbengfort bbengfort self-assigned this Feb 23, 2017
@coveralls
Copy link

coveralls commented Feb 23, 2017

Coverage Status

Coverage remained the same at 69.348% when pulling 7f440dc on bbengfort:develop into 98829fb on DistrictDataLabs:develop.

@NealHumphrey
Copy link
Contributor

NealHumphrey commented Feb 23, 2017

@bbengfort I was thinking more that while external contributors are free to use whatever method they want (modifying master, etc), if anyone asks what to do or if we want to put any suggestions into the CONTRIBUTING.MD, we could recommend they make a feature branch within their fork. As you said last night, making feature branches just makes the process cleaner, and while not as necessary as it is in a centralized team repo it's still helpful in triangular workflows.

@bbengfort bbengfort changed the title examples refactor to user notebooks examples refactor to user notebooks connects with #129 Feb 27, 2017
@coveralls
Copy link

coveralls commented Feb 27, 2017

Coverage Status

Coverage remained the same at 69.348% when pulling 448889a on bbengfort:develop into 8c03265 on DistrictDataLabs:develop.

@coveralls
Copy link

coveralls commented Feb 27, 2017

Coverage Status

Coverage remained the same at 69.348% when pulling ad044d0 on bbengfort:develop into 8c03265 on DistrictDataLabs:develop.

@bbengfort
Copy link
Member Author

@NealHumphrey @rebeccabilbro -- I'm ready to merge this PR if one of you would like to review. I've completed #129 and #134 in this pull request.

@bbengfort bbengfort added review PR is open and removed in progress label for Waffle board labels Feb 27, 2017
@coveralls
Copy link

coveralls commented Feb 27, 2017

Coverage Status

Coverage remained the same at 69.348% when pulling 44d14ab on bbengfort:develop into 8c03265 on DistrictDataLabs:develop.

@rebeccabilbro rebeccabilbro merged commit 10124a5 into DistrictDataLabs:develop Feb 28, 2017
@rebeccabilbro rebeccabilbro removed the review PR is open label Feb 28, 2017
@bbengfort
Copy link
Member Author

@rebeccabilbro - nice thanks; did you see the code review process? There is a place where you can review the commit and approve or request changes. Once the code is reviewed, it can be automatically merged. I'm actually really liking the new GitHub contributors workflow.

@rebeccabilbro
Copy link
Member

@bbengfort hmm, I reviewed your commits and then used the squash/merge button -- is that what you're referring to? I think that was how we did it at PyCon last year, too; is there something new I'm missing? Perhaps it would be best for us to do a walkthrough before or during the next sprint review. I can meet up early on 3/8 if you want.

@bbengfort
Copy link
Member Author

bbengfort commented Feb 28, 2017

@rebeccabilbro - yes, there is a new code review mechanism that goes hand-in-hand with the protected branch stuff. Your icon is yellow on the PR, so I'm guessing that the squash/merge button is different. Let's definitely do a walkthrough before or after the next review. Not sure of my schedule quite yet.

@rebeccabilbro
Copy link
Member

@bbengfort sounds good; let me know.

rebeccabilbro pushed a commit that referenced this pull request Mar 29, 2017
* add my user test for #121

* examples README

* examples refactor to user notebooks connects with #129 (#135)

* examples refactor to user notebooks

* text frequency visualizer

* added text examples to examples notebook

* completed user study

* added mushroom dataset for case study

* contributing read me and throughput graph

* added feature_visualizer

* preliminary implementation of PosTagVisualizer for #150

* fixed #138

* added notebook with alphas examples

* Initial prototype of alpha selection visualizer
Also refactored the regressor module and documentation.

* finished up alphas selection visualizer

* add energy dataset

* added jointplot.ipynb

* parametrized colormap for hex plot

* added more features and added documentation

* cleanup

* added more documentation and organized functions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task non-code related task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants