10000 ENH Add verbose option to SpectralClustering by sstalley · Pull Request #18052 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

ENH Add verbose option to SpectralClustering #18052

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Aug 7, 2020
Merged

ENH Add verbose option to SpectralClustering #18052

merged 6 commits into from
Aug 7, 2020

Conversation

sstalley
Copy link
Contributor

Added verbose option to spectral_clustering and SpectralClustering, making issues easier to debug.

Resolves #17969.

Any other comments?

This is my first pull request for sklearn (as well as my first pull request using github),
so any feedback or constructive criticism is appreciated. Thanks!

TomDLT
TomDLT approved these changes Jul 31, 2020
View reviewed changes
< 10000 /span> Copy link
Member
@TomDLT TomDLT left a comment

Choose a reason for hiding this comment

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

Looks good to me.

You can also add a versionadded tag in the docstring (examples).

thomasjpfan
thomasjpfan reviewed Aug 1, 2020
View reviewed changes
Copy link
Member
@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @sstalley !

thomasjpfan
thomasjpfan approved these changes Aug 6, 2020
View reviewed changes
Copy link
Member
@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Please add an entry to the change log at doc/whats_new/v0.24.rst with tag |Enhancement|. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.

@thomasjpfan thomasjpfan changed the title ENH Add verbose option to SpectralClustering (#17969) ENH Add verbose option to SpectralClustering Aug 7, 2020
@sstalley
Copy link
Contributor Author
sstalley commented Aug 7, 2020

@thomasjpfan the only other potential author would be you (for the f-string formatting in b9c7af8).
I didn't attribute you for this in the documentation, but can add your name if you like.

Also, I wasn't sure of the best way to resolve the merge conflict. I chose to rebase - but now I see that messes up the discussion history in the PR. Is there a preferred method of resolving merge conflicts for this project?

@thomasjpfan
Copy link
Member
thomasjpfan commented Aug 7, 2020 •
edited
Loading

I didn't attribute you for this in the documentation, but can add your name if you like.

We normally do not attribute the reviewers, so there is no need to add my name.

Also, I wasn't sure of the best way to resolve the merge conflict. I chose to rebase - but now I see that messes up the discussion history in the PR. Is there a preferred method of resolving merge conflicts for this project?

We prefer to merge with master and handle the conflicts there. When we merge with master, we will square the commits.

For this PR, I pushed to this branch to fix the conflicts and adjust the grammar a little and was about to merge.

Edit: I left a review with the changes.

thomasjpfan
thomasjpfan approved these changes Aug 7, 2020
View reviewed changes
@sstalley @thomasjpfan
Improve wording in doc/whats_new/v0.24.rst
7a3bc3b 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@sstalley
Copy link
Contributor Author
sstalley commented Aug 7, 2020

We prefer to merge with master and handle the conflicts there. When we merge with master, we will square the commits.
For this PR, I pushed to this branch to fix the conflicts and adjust the grammar a little and was about to merge.

Thanks for the explanation. From now on I will ignore conflicts if the code has already been approved. Sorry for getting in your way this time - I'll try not to let it happen again.

@thomasjpfan
Copy link
Member

From now on I will ignore conflicts if the code has already been approved. Sorry for getting in your way this time - I'll try not to let it happen again.

Don't worry about it! It is good to fix the conflicts. In this case, we just so happened to be working on it at the same time. :)

@thomasjpfan thomasjpfan merged commit 603d05b into scikit-learn:master Aug 7, 2020
@thomasjpfan
Copy link
Member

Thank you for working on this @sstalley !

@sstalley
Copy link
Contributor Author
sstalley commented Aug 7, 2020

Thank you for working on this < 8000 a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/sstalley/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://github.com/sstalley">@sstalley !

Of course. Thanks for your help & review @thomasjpfan !

jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@sstalley @thomasjpfan @jayzed82
ENH Add verbose option to SpectralClustering (scikit-learn#18052)
535176e 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@TomDLT TomDLT TomDLT approved these changes

Assignees
No one assigned
Labels
module:cluster Waiting for Reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Verbose debugging for spectral clustering
4 participants
@sstalley @thomasjpfan @TomDLT @cmarmo
0