Skip to content

[MRG] EXA align lorenz curves between the two examples with GLMs #16640

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

Merged
merged 2 commits into from
Mar 5, 2020

Conversation

lorentzenchr
Copy link
Member

Reference Issues/PRs

#14300 added 2 examples, one for Poisson, one for Tweedie regression. In the last plot of each example, a lorenz curve shown. But one orders the x-axis opposite to the other example.

What does this implement/fix? Explain your changes.

This PR aligns the x-axis of the last plot, always from safest to riskiest policies.

@lorentzenchr lorentzenchr changed the title EXA align lorenz curves between the two examples with GLMs [MRG] EXA align lorenz curves between the two examples with GLMs Mar 4, 2020
Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

@rth rth merged commit df338cd into scikit-learn:master Mar 5, 2020
@rth
Copy link
Member

rth commented Mar 5, 2020

I don't get why I'm marked as author in commits for two last PRs that I merged. I have done everything as usual (i.e. clicking on the merge commit button). Either it's a Github bug or they are changing something. We'll see what happens for other PRs merged today.

@tqchen
Copy link

tqchen commented Mar 5, 2020

FYI, we are also seeing this problem in some of our repos. The merging author does seems to be a github problem.

@tqchen
Copy link

tqchen commented Mar 5, 2020

We just sent a bug report to the github support. Perhaps all related project should do so to escalate the issue.

@rth
Copy link
Member

rth commented Mar 5, 2020

Thanks for the confirmation! I also contacted Github support. This seem related to https://github.community/t5/How-to-use-Git-and-GitHub/Authorship-of-merge-commits-made-by-Github-Apps-changed/m-p/48797#.

@rth
Copy link
Member

rth commented Mar 5, 2020

Response from Github suport:

Thanks for writing in about this!

This is an intentional change that our team recently deployed and I'm sorry to hear that's causing trouble.When a user chooses to squash and merge a PR they are taking authorship of the commit. If they were merging the PR and creating a merge commit that merge commit would be attributed to them as author, so we wanted to be consistent with that hence the change. We are also aware that we should be setting the author as a co-author and we can see that's not happening every time.

We've passed this on to our team both as feedback about the recent changes and to look into why co-authors are not being set as expected.
[..]

It's possible that I had removed the Co-authored-by tag by accident in this case.

@scikit-learn/core-devs FYI, when you next merge PR please make sure you keep Co-authored-by tag in the commit description for the PR author.

@greysteil
Copy link

FYI, this issue (the change in commit author) got escalated to me at GitHub. We have a bug in our squash and merge logic right now (introduced yesterday) which causes the original PR author to be removed from the list of commit co-authors in some cases. We're working on a fix now.

(The change to make the merger an author will persist, as that is what git merge --squash uses, but the loss of the co-authored-by in the auto-generated commit message is a bug - sorry about that!)

@tqchen
Copy link

tqchen commented Mar 5, 2020

Thanks @greysteil for working on this. Is it possible to give an option to retain the old behavior?

Personally I feel that the old squash merge behavior is better. The new way gives additional stats to the merger and can be confusing when looking into the commit history of maintainers.

We could go with the rebase merge. But one thing that we really liked about the original squash merge is that it directly generates commit messages that links to the PR.

Would love to see if there is a way for community to choose the behavior they want

@greysteil
Copy link

Thanks for the feedback - we're going to revert the default behaviour back to the previous, PR-creator-as-author, setup. The fix will go out in the next 24 hours and we'll think of another way to address the problems the change was intended to solve.

@u99127
Copy link

u99127 commented Mar 5, 2020

Good to hear this is being rolled back. However, there is a point that I haven't seen come up in this discussion which I think is important. Keeping exact records of authorship information in the commit is really important. I feel that merging that with co-authorship is not entirely being accurate as not every contributor has commit rights in the github projects and different projects have different maintainer structures. It is important to keep accurate records of authorship as projects need to be aware of who authored what, so why is this needed ? This is needed in case there is a requirement for relicensing or any other query regarding the authorship and yes that does happen in a long running project that uses git as a version control system and github as a hosting system.

My 2 cents.

Thanks,
Ramana

@lorentzenchr lorentzenchr deleted the GLM-reverse-axis branch March 5, 2020 20:11
@thomasjpfan thomasjpfan mentioned this pull request Mar 5, 2020
ashutosh1919 pushed a commit to ashutosh1919/scikit-learn that referenced this pull request Mar 13, 2020
gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020
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.

5 participants