Skip to content

MAINT: Replace numpy custom generation engine by raw C++ - #19713

Merged
mattip merged 1 commit into
numpy:mainfrom
serge-sans-paille:feature/to-cxx-and-beyond
Oct 22, 2021
Merged

MAINT: Replace numpy custom generation engine by raw C++#19713
mattip merged 1 commit into
numpy:mainfrom
serge-sans-paille:feature/to-cxx-and-beyond

Conversation

@serge-sans-paille

@serge-sans-paille serge-sans-paille commented Aug 19, 2021

Copy link
Copy Markdown
Contributor

This is just a technical prototype to measure and discuss the impact and
implication of moving to C++ for kernel code generation.

@serge-sans-paille
serge-sans-paille force-pushed the feature/to-cxx-and-beyond branch from 848d64f to bafb152 Compare August 19, 2021 07:51
@eric-wieser

eric-wieser commented Aug 19, 2021

Copy link
Copy Markdown
Member

This looks great to me, but I don't know how easy it will be to get this working in our build system.

@serge-sans-paille
serge-sans-paille force-pushed the feature/to-cxx-and-beyond branch from bafb152 to a0a48ce Compare August 19, 2021 12:30
@rgommers rgommers added the C++ Related to introduction and use of C++ in the NumPy code base label Aug 19, 2021
@rgommers

rgommers commented Aug 19, 2021

Copy link
Copy Markdown
Member

Interesting, thanks @serge-sans-paille!

I don't know how easy it will be to get this working in our build system.

It looks like it builds okay on Azure, there are just a few failures. On TravisCI I see:

cc1plus: warning: ‘-Werror=’ argument ‘-Werror=implicit-function-declaration’ is not valid for C++

-Werror is added via runtests.py --warn-error in CI only, should be easy to fix.

Not sure what the exact problem is in cygwin_build_test. I had totally missed that we have a Cygwin CI job now.

Do you expect specific issues @eric-wieser?

@serge-sans-paille
serge-sans-paille force-pushed the feature/to-cxx-and-beyond branch from a0a48ce to 5016080 Compare August 19, 2021 13:34
@eric-wieser

eric-wieser commented Aug 19, 2021

Copy link
Copy Markdown
Member

Do you expect specific issues @eric-wieser?

No, but I think I've seen build/distribution-issue-based resistance to switching to c++ before, and am merely expecting someone else to bring those up again. I don't know the details I'm afraid.

@rgommers

rgommers commented Aug 19, 2021

Copy link
Copy Markdown
Member

Okay thanks. I think there are likely to be some fiddly details, but given that numpy.distutils supports C++ just fine and it works for SciPy, I would not expect any really fundamental problems.

@serge-sans-paille
serge-sans-paille force-pushed the feature/to-cxx-and-beyond branch from 5016080 to a6982ad Compare August 19, 2021 14:19
peterbell10
peterbell10 reviewed Aug 19, 2021
View reviewed changes
Comment thread numpy/core/src/umath/clip.cpp Outdated
@charris

charris commented Aug 19, 2021

Copy link
Copy Markdown
Member

and am merely expecting someone else to bring those up again.

We discussed that in one of the triage meetings and decided modern C++ was acceptable, @seiko2plus wanted it.

@serge-sans-paille
serge-sans-paille force-pushed the feature/to-cxx-and-beyond branch 4 times, most recently from 5cd48a6 to d401263 Compare August 20, 2021 07:31
@serge-sans-paille

serge-sans-paille commented Aug 20, 2021

Copy link
Copy Markdown
Contributor Author

All tests green except cygwin, and there's no error log so I can't track the error correctly, if some knows more than me about it ?

@serge-sans-paille
serge-sans-paille force-pushed the feature/to-cxx-and-beyond branch 14 times, most recently from 7dbc175 to a9d5772 Compare August 21, 2021 20:33
@serge-sans-paille
serge-sans-paille force-pushed the feature/to-cxx-and-beyond branch 8 times, most recently from 3fa3dc5 to 3a59afd Compare October 22, 2021 09:25
@serge-sans-paille
[demo] how-to replacing numpy custom generation engine by raw C++
2ae7aeb 
This is just a technical prototype to measure and discuss the impact and
implication of moving to C++ for kernel code generation.
@serge-sans-paille
serge-sans-paille force-pushed the feature/to-cxx-and-beyond branch from 3a59afd to 2ae7aeb Compare October 22, 2021 09:57
@serge-sans-paille

serge-sans-paille commented Oct 22, 2021

Copy link
Copy Markdown
Contributor Author

And all green again ;-)

@mattip

mattip commented Oct 22, 2021

Copy link
Copy Markdown
Member

In the recent developer meeting, we decided to put this in as an experimental enhancement, without actually pushing to migrate the templated code to c++ templates for the 1.22 release. That way we can see if the infrastructure is solid before progressing.

@mattip
mattip merged commit 53d2a83 into numpy:main Oct 22, 2021
@mattip

mattip commented Oct 22, 2021

Copy link
Copy Markdown
Member

Thanks @serge-sans-paille

@charris charris changed the title [demo] how-to replacing numpy custom generation engine by raw C++ MAINT: Replace numpy custom generation engine by raw C++ Oct 23, 2021
@SylvainCorlay

SylvainCorlay commented Oct 25, 2021

Copy link
Copy Markdown

🎉

@seiko2plus seiko2plus mentioned this pull request Oct 27, 2021
Merged
@seiko2plus seiko2plus mentioned this pull request Nov 10, 2021
Closed
@serge-sans-paille serge-sans-paille mentioned this pull request Dec 2, 2021
Merged
@aragilar aragilar mentioned this pull request Jan 7, 2022
Closed
rgommers added a commit to rgommers/numpy that referenced this pull request Jan 24, 2022
@rgommers
BUG: distutils: fix building mixed C/Fortran extensions
1e83f83 
In SciPy we had a couple of cases where we build a Python extension
with C source files but linked against static libraries built from
Fortran code. Those should be using the Fortran linker, but this
was broken in 1.22.0 by numpygh-19713 (the PR that introduced C++ in NumPy).

This fixes a few issues in the `build_ext` command, and documents better
what is going on there.

Should close SciPy issues 8325 and 15414.
@rgommers rgommers mentioned this pull request Jan 24, 2022
Merged
charris pushed a commit to charris/numpy that referenced this pull request Jan 27, 2022
@rgommers @charris
BUG: distutils: fix building mixed C/Fortran extensions
da48521 
In SciPy we had a couple of cases where we build a Python extension
with C source files but linked against static libraries built from
Fortran code. Those should be using the Fortran linker, but this
was broken in 1.22.0 by numpygh-19713 (the PR that introduced C++ in NumPy).

This fixes a few issues in the `build_ext` command, and documents better
what is going on there.

Should close SciPy issues 8325 and 15414.
@charris charris mentioned this pull request Jan 27, 2022
Merged
@jschueller jschueller mentioned this pull request Jan 28, 2022
Closed
@serge-sans-paille serge-sans-paille mentioned this pull request Mar 21, 2022
Merged
@mattip mattip mentioned this pull request Apr 5, 2022
Closed
@scratchmex scratchmex mentioned this pull request Aug 25, 2022
Open
@rkern rkern mentioned this pull request Apr 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seberg seberg seberg left review comments
@pearu pearu pearu left review comments
@mattip mattip mattip left review comments
+2 more reviewers
@peterbell10 peterbell10 peterbell10 left review comments
@fawazfzk fawazfzk fawazfzk approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

03 - Maintenance C++ Related to introduction and use of C++ in the NumPy code base

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.