wlav / cppyy

Other
384 stars 38 forks source link

Documenting Numba Enhancements contributed by Baidyanath #199

Closed QuillPusher closed 5 months ago

QuillPusher commented 8 months ago
QuillPusher commented 8 months ago

Hello @wlav ,

I'm working as a Technical Writer for the Compiler Research Org. I had a few information sessions with Baidyanath and Vassil, based on which I have created this document.

The examples/demos have been copied from Baidyanath's PyHep22 Presentation.

wlav commented 8 months ago

There should be no mention of PyROOT in cppyy documentation. The ROOT team forked cppyy at some point in the long past and it's a completely separate product. It should not be documented here. (Also things like ROOT.Numba.Declare aren't/won't supported in this way.)

Adding Numba support wasn't just provided by the CaaS project: I brought it up to the point that the given examples of the original documenation ran. Baidyanath work was on top of that, in particular the improvements in overload selection (as well as the port back into PyROOT). There have been several improvements done by Aaron (through GSoC). The latter also means that for the code, the main repo is the right place: the link provided in the CaaS repo is to an out-of-date file.

In particular, this "For now, Numba supports primitive types and some class types in C++, but support for other core C++ features (e.g., reference types) is still in the works." has been implemented by Aaron.

This: "Among other things, the numba_ext.py file includes manual C++ type conversions (_cpp2numba). This manual mapping is included since Numba doesn't inherently capture C++ types (as opposed to Python types, which it does support)." is neither true nor relevant as it's an implementation detail (same for _cpp2ir), not part of a public API that a developer can add, to. It was also changed by Aaron b/c the original implementation was too naive.

This: "Numba doesn't know the overload of the function before the user inputs their arguments." comes with the qualifier that Numba allows you to add annotations to the decorator.

What's the reason for re-formatting the original code samples? In particular, the interactive use as shown gives better spacing and shows the output lined up with the input. (Code-blocks should also be identified as Python, given that its a mix of Python and C++ which sometimes confuses the doc script.)

Speaking about formatting, it would be great to have a line break after each sentence. It doesn't matter for the formatting in the final product, but makes editing later much easier.

This document as-is, was mostly for showing some examples. An upgraded version should include a section to motivate the work. There are two prime motivations: 1) to allow kernels for C++ frameworks (in particular GPU ones, although we're not there yet) be written in Python; and 2) performance. Most of the document focuses on the latter, which is fine, but the former is the more interesting one, especially as it comes to the outlook.

QuillPusher commented 8 months ago

@wlav , thanks for the review and clarifications, I'll update these shortly

QuillPusher commented 8 months ago

Incorporated Wim's review changes and squashed commits for better readability:

QuillPusher commented 7 months ago

Hello @wlav , hope you're doing well (:

Please let me know if the draft looks better now (addressed changes listed in previous comment), or share further feedback to help fill any gaps

wlav commented 7 months ago

There are a few things still there that should not be documented publicly. In particular, _cpp2ir, CppFunctionNumbaType, etc. These are internal details that are bound to change after this first implementation. Documenting them publicly gives the impression they will be available and supported, which may not be the case.

I can approve the pull request and remove these myself, though.

QuillPusher commented 6 months ago

Sounds good, please feel free to update these and any other details, since you have the best insights in this matter (: (apologies for the late response, I was on sick leave)

QuillPusher commented 6 months ago

Hello @wlav, please let me know if there's something pending at my end.

If its just internal details like _cpp2ir, CppFunctionNumbaType, etc., please update as needed to help finalize this

wlav commented 5 months ago

Merging first; easier to clean up directly.