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 paper.md, mailmap and small URL fix #359

Merged
merged 5 commits into from
Feb 17, 2020
Merged

Update paper.md, mailmap and small URL fix #359

merged 5 commits into from
Feb 17, 2020

Conversation

titipata
Copy link
Collaborator

  • Update the text in paper.md
  • Fix a few typos in the repository

@titipata titipata requested a review from jasmainak February 17, 2020 14:44
@codecov-io
Copy link

codecov-io commented Feb 17, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #359   +/-   ##
=======================================
  Coverage   75.66%   75.66%           
=======================================
  Files           4        4           
  Lines         678      678           
  Branches      149      149           
=======================================
  Hits          513      513           
  Misses        128      128           
  Partials       37       37

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 0669e72...17cfb07. Read the comment docs.

@titipata
Copy link
Collaborator Author

titipata commented Feb 17, 2020

@jasmainak @pavanramkumar I fixed some text and typos in paper and a few places in documentation. I think this should addresses the comments in JOSS review.

@pavanramkumar
Copy link
Collaborator

thank you @titipata! i added some minor wordsmithing changes. @jasmainak looks good to merge for me!

@titipata
Copy link
Collaborator Author

thanks for editing @pavanramkumar! After this PR, it should be ready for JOSS submission. The writing is much better now.

paper/paper.md Outdated

# Example Usage

Here we apply pyglmnet to a real-world example. The Community and Crime dataset, one of 400+ datasets curated by the UC Irvine Machine Learning Repository [@Dua:2019] provides a highly curated set of 128 demographic attributes of US counties that may be used to predict incidence of violent crime. The target variable (violent crime per capita) is normalized to lie in $[0, 1]$. Below, we demonstrate the usage of a binomial-distributed GLM with elastic net regularization.
Here, we apply pyglmnet to predict incidence of violent crime from the Community and Crime dataset, one of 400+ datasets curated by the UC Irvine Machine Learning Repository [@Dua:2019] provides a highly curated set of 128 demographic attributes of US counties. The target variable (violent crime per capita) is normalized to the range of $[0, 1]$. Below, we demonstrate the usage of a pyglmnet's binomial-distributed GLM with elastic net regularization.
Copy link
Member

Choose a reason for hiding this comment

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

The first sentence reads weird. We're missing a "which"?

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, it should be "which provides". Can you do a quick edit on that?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I will to do that.

Btw, note for the future -- it is not a good idea to make PRs from master. Maybe we should add it to our contributing guide. Because as the repository owner, if I push to your master then your master on origin is not a simple fast forward merge from upstream master. So if you accidentally pull from origin instead of upstream, you may have a bad master branch locally. Also, if I force push to this branch, it can close the pull request (recent experience of a lab colleague).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jasmainak nevermind, I did a fix according to your comment!

Copy link
Member

Choose a reason for hiding this comment

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

lol okay, we don't need to bother about the story above then :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jasmainak Yeah, totally agree, I shouldn't make PR from the master branch! I will make PR from my branches for the future PRs.

@jasmainak jasmainak merged commit 1c8b834 into glm-tools:master Feb 17, 2020
@jasmainak
Copy link
Member

Merged, thanks @titipata and @pavanramkumar !

@titipata
Copy link
Collaborator Author

@jasmainak @pavanramkumar should we tag the reviewer and editor in JOSS to do a final review then?

@jasmainak
Copy link
Member

@titipata I am going to try to see if I can improve the example in the readme. One of the reviewers mentioned it as a "good to have". Then we can reply to the editor in the JOSS issue :) Will try to get it done by tonight.

@titipata
Copy link
Collaborator Author

@jasmainak Awesome! I saw the comment on that. Feel free to tag me to review the PR :)

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