8000 [MRG + 1] MAINT Move heapify_up/heapify_down into PriorityHeap as class methods + COSMITs by nelson-liu · Pull Request #7034 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

[MRG + 1] MAINT Move heapify_up/heapify_down into PriorityHeap as class methods + COSMITs #7034

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 7 commits into from
Dec 27, 2016

Conversation

nelson-liu
Copy link
Contributor

I've seen a lot of misc style / refactoring sort of issues while working on the tree code. Rather than clutter up other PRs with these changes, i figured that i would move them here.

What does this implement/fix? Explain your changes.

  • removes several trailing whitespaces
  • moves the heapify_up and heapify_down methods to be within the PriorityHeap structure
  • general spelling fixes

@nelson-liu nelson-liu changed the title RFC: Misc style / refactoring of tree module [WIP] RFC: Misc style / refactoring of tree module Jul 17, 2016
@amueller
Copy link
Member

is this still relevant and do you want reviews?

@nelson-liu
Copy link
Contributor Author

it's relevant, but let's wait on reviews ;) this is just a place to put a bunch of misc changes. thanks for offering, though!

@raghavrv
Copy link
Member

moves the heapify_up and heapify_down methods to be within the PriorityHeap structure

+1 for that. Could you revive the PR and make travis pass?

Thanks!

@nelson-liu
Copy link
Contributor Author

@raghavrv done, thanks for reminding me about this.

@nelson-liu nelson-liu changed the title [WIP] RFC: Misc style / refactoring of tree module [MRG] RFC: Misc style / refactoring of tree module Oct 24, 2016
@raghavrv raghavrv changed the title [MRG] RFC: Misc style / refactoring of tree module [MRG] MAINT Move heapify_up/heapify_down into PriorityHeap as class methods + COSMITs Oct 24, 2016
@@ -240,6 +206,38 @@ cdef class PriorityHeap:
cdef bint is_empty(self) nogil:
return self.heap_ptr <= 0

cdef void heapify_up(self, PriorityHeapRecord* heap, SIZE_t pos) nogil:
Copy link
Member

Choose a reason for hiding this comment

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

@jmschrei @glouppe This looks cleaner to me. Are you fine with this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I originally moved it in because I made another priorityheap class for use with non-records, but we ended up deciding that was too slow. I do think it is cleaner, though.

@nelson-liu
Copy link
Contributor Author
< 8000 /tr>

gentle ping wrt this?

@raghavrv
Copy link
Member

+1 from me another review from @jmschrei?

@raghavrv raghavrv changed the title [MRG] MAINT Move heapify_up/heapify_down into PriorityHeap as class methods + COSMITs [MRG + 1] MAINT Move heapify_up/heapify_down into PriorityHeap as class methods + COSMITs Dec 22, 2016
@jnothman
Copy link
Member

This looks fine to me, but I wouldn't mind a quick check from one of you tree loppers.

@raghavrv
Copy link
Member
raghavrv commented Dec 27, 2016

I think a +1 from you will suffice :) Merging as most (other) tree loopers are quite bust atm and I've bigger PRs for them ;)

@raghavrv raghavrv merged commit f95e5b1 into scikit-learn:master Dec 27, 2016
sergeyf pushed a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017
…ss methods + COSMITs (scikit-learn#7034)

* RFC: move heap methods to class and remove trailing spaces

* spurious comment to force recythonization of boosting

* [ci skip] remove spurious comment

* remove trailing whitespace on line

* style: fix trailing whitespace in _criterion.pxd

* add spurious comments to try to force recythonizing

* remove changes for recythonization
@Przemo10 Przemo10 mentioned this pull request Mar 17, 2017
Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
…ss methods + COSMITs (scikit-learn#7034)

* RFC: move heap methods to class and remove trailing spaces

* spurious comment to force recythonization of boosting

* [ci skip] remove spurious comment

* remove trailing whitespace on line

* style: fix trailing whitespace in _criterion.pxd

* add spurious comments to try to force recythonizing

* remove changes for recythonization
NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017
…ss methods + COSMITs (scikit-learn#7034)

* RFC: move heap methods to class and remove trailing spaces

* spurious comment to force recythonization of boosting

* [ci skip] remove spurious comment

* remove trailing whitespace on line

* style: fix trailing whitespace in _criterion.pxd

* add spurious comments to try to force recythonizing

* remove changes for recythonization
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
…ss methods + COSMITs (scikit-learn#7034)

* RFC: move heap methods to class and remove trailing spaces

* spurious comment to force recythonization of boosting

* [ci skip] remove spurious comment

* remove trailing whitespace on line

* style: fix trailing whitespace in _criterion.pxd

* add spurious comments to try to force recythonizing

* remove changes for recythonization
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
…ss methods + COSMITs (scikit-learn#7034)

* RFC: move heap methods to class and remove trailing spaces

* spurious comment to force recythonization of boosting

* [ci skip] remove spurious comment

* remove trailing whitespace on line

* style: fix trailing whitespace in _criterion.pxd

* add spurious comments to try to force recythonizing

* remove changes for recythonization
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
0