DOC Add a 'Cython Best Practices, Conventions and Knowledge' section

[jjerphan]

Reference Issues/PRs

What does this implement/fix? Explain your changes.

This adds a section to scikit-learn documentation regarding some Cython know-how of the team.

Any other comments?

Feel free to suggest changes, adaptations or additions.

[lorentzenchr]

@jakirkham might be interested.

[ArturoAmorQ]

A first comment before a proper review: I think it would be nice to point towards this new section from the reading the existing code base and the performance tips for the cython developer sections. It could be a simple "for more information, see section ...".

[jjerphan]

@ArturoAmorQ: feel free to open a pull-request on my branch if you think this can be better worded and if it is easier for you. 👍

[jjerphan]

Relevant notes from @da-woods people might be interested reading: https://cython-guidelines.readthedocs.io/en/latest/

[ArturoAmorQ]

@ArturoAmorQ: feel free to open a pull-request on my branch if you think this can be better worded and if it is easier for you. +1

You can cherrypick my commit here: ArturoAmorQ/scikit-learn@32648c5 (#2)

[jeremiedbb]

It's something I would have liked to read when I joined the project, thanks for working on this. Here are some comments.

In addition in the performance page, there's this old TODO
TODO: html report, type declarations, bound checks, division by zero checks, memory alignment, direct blas calls...
I think this PR is a good opportunity to finally do it :)
In particular, I think it's worth adding a bullet point about direct calls to blas: when it's desirable, must be through the cython_blas extension... The rest is kind of already done I think

[jjerphan]

@da-woods: how interested are you to merge such notes (with yours) (in Cython's documentation)?

[adam2392]

I much enjoy and appreciate the snippets of code I can just copy and paste.

[da-woods]

@da-woods: how interested are you to merge such notes (with yours) (in Cython's documentation)?

I think we are interesting in merging more of this sort of this into the Cython documentation. It probably won't be all my notes - I know there's a few of my opinions (e.g. my abiding dislike of cpdef functions) that Stefan doesn't necessarily agree with so I'd rather keep those somewhere where it's "just my opinion".

I still think duplication doesn't hurt so there's no reason not have this section in PandasScikit-learn and then see what we can pick across.

[jjerphan]

I tend to agree with all of your notes.

While it is useful to have Cython's developers and users (e.g. you and me here) share their experience, tips and conventions about the language, I think everyone would benefit from having them unified at one canonical place (ideally Cython's docs).

Do you think it possible to identify and extract non-opiniated, factual elements from both yours and those notes and integrates them in Cython's docs ?

Also, were you referring to scikit-learn instead of pandas in your previous comments ?

[da-woods]

Do you think it possible to identify and extract non-opiniated, factual elements from both yours and those notes and integrates them in Cython's docs ?

I'll try to make a start on that in the next few weeks (probably bit-by-bit)

Also, were you referring to scikit-learn instead of pandas in your previous comments ?

Oops yes sorry! I got temporarily dislocated!

[jeremiedbb]

Another pass, mostly regarding the rendering this time.

[betatim]

Added the "no changelog" label, as I think this wouldn't get added.

I like it and find it useful as is. As such, should we try and merge this as soon as this round of comments is addressed? It seems like work on this PR could go on for a long time because there is so much to know. However the longer it is unmerged, the longer people won't profit from what is already here. Can always do a second, third, fourth PR to keep working on this. WDYT?

[jjerphan]

Yes, I agree. I think Jérémie is busy with the 1.2.2 release, but other maintainers can give their +1.

I am waiting for this PR to get 2 approvals with potential proposals for subsequent PRs improving the section.

[jeremiedbb]

Can always do a second, third, fourth PR to keep working on this

I'm also in favor of merging a good enough version and improve in subsequent PRs

[jjerphan]

@jeremiedbb: I think I've treated all of your comments.

[jeremiedbb]

Reading the discussions in #25780, I think it will be worth adding a bullet point about including cnp.import_array(), but it can be done in another PR

[lorentzenchr]

I like it and find it useful as is. As such, should we try and merge this as soon as this round of comments is addressed? It seems like work on this PR could go on for a long time because there is so much to know. However the longer it is unmerged, the longer people won't profit from what is already here. Can always do a second, third, fourth PR to keep working on this. WDYT?

I agree 💯 It is better to have 4 smaller PRs and to iterate faster.

[lorentzenchr]

Let‘s iterate.

[jeremiedbb]

LGTM. There's just this rendering issue that bothers me :/

[jeremiedbb]

last nitpick. The extra indentation made the first sentence bold, but all other bullet points don't so let's remove it for consistency

[jeremiedbb]

Thanks

[jjerphan]

Thanks everyone! I hope this will help people getting started using Cython for scikit-learn.

[da-woods]

FWIW I have an PR that should dramatically improve this. It almost certainly won't (and shouldn't!) make it into Cython 3, but hopefully won't be long after that. Essentially, a lot of the reference counting turns out to be pretty pointless since you're usually incrementing and decrementing exactly the same object so it can be skipped completely.

So hopefully this advice can change sometime in the next year. But not yet

[jjerphan]

Thank you for mentioning this, @da-woods.