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

MAINT: Refactor sorting header file #12366

Merged
merged 1 commit into from
Nov 11, 2018
Merged

MAINT: Refactor sorting header file #12366

merged 1 commit into from
Nov 11, 2018

Conversation

liwt31
Copy link
Contributor

@liwt31 liwt31 commented Nov 11, 2018

This is part of adding timsort to numpy sorting algorithms #12186.
While trying to modify the header files related to sorting algorithms I find numpy/core/src/common/npy_sort.h is verbose and hard-coded. It might be a better idea to use a template numpy/core/src/common/npy_sort.h.src (the main part of this PR) to generate the file when building numpy.
BTW some lines in .gitignore labeled as # Things specific to this project # seem outdated. Maybe this should be filed in another issue?

@liwt31
Copy link
Contributor Author

liwt31 commented Nov 11, 2018

I assume the codecov report isn't making lots of sense here?

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Looks great, thanks. I wonder why this was not done like this to begin with...

codecov failure is garbage, and due to setup.py not having any coverage data, even though those lines are obviously executing in order to produce the build

@charris charris merged commit 5c9c688 into numpy:master Nov 11, 2018
@charris
Copy link
Member

charris commented Nov 11, 2018

Thanks @liwt31 .

@charris
Copy link
Member

charris commented Nov 11, 2018

Looks great, thanks. I wonder why this was not done like this to begin with...

The include file was made a couple of years before we started generating include files. The idea of generating it didn't even occur to me...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants