sisl icon indicating copy to clipboard operation
sisl copied to clipboard

Move to Pure Python syntax for Cython

Open pfebrer opened this issue 2 years ago • 8 comments

When trying to optimize the DM calculation, I ran into the fact that you can use python's type annotations instead of cython special syntax to generate equally efficient C code (Docs).

Did you already know this?

To me it looks much better since you don't have to write/learn a different syntax (things look more familiar and therefore easier to understand) and also the same code can run in the normal python interpreter if cython is missing. The pure python syntax seems to be the expected dominant way of using cython in the future. Also, I'm sure other options for optimization through static types will arise in the future and it will be much easier to port to them in case it is needed if sisl uses python syntax.

What do you think?

pfebrer avatar Jun 15 '22 09:06 pfebrer

I didn't know this, no. Thanks for bringing this up.

  1. If it should fall back to regular Python execution, then lots of boilerplate code needs to be added to remove cython imports and specific annotations
  2. How does the type-annotation work for specifying the dimensionality and data-layout of numpy.ndarray 's ? Currently the Cython implementions I did have specific requirements for the data-layout which helps the compiler optimize things; i.e. I tell it to get a matrix in row-order, and not a matrix in column order. These things are important for vectorizations. It seems to me that type-annotations don't allow this to be distinguished?
  3. Does the type-annotations allow ctypedef fused? It probably does, but it should before even thinking about it ;)

I am not sure that Cython is pushing this very much? Where did you find this statement? My main concern is the points 2 and 3, which I don't think can be annotated as it is.

If you want to push this, consider making an example of _matrix_phase.pyx which isn't too long, but has some of the required things. ;)

zerothi avatar Jun 15 '22 18:06 zerothi

  1. They say that in that case they have a placeholder, which is just a file that contains all cython namespace but does nothing: http://docs.cython.org/en/latest/src/tutorial/pure.html#magic-attributes. It is in the cython module, but can be copied to sisl. However, I would say that the code won't be expected to be run in the python interpreter directly anyway since loops will be much slower than numpy. I would say the main advantages are that it's faster to go from pure-python code to cython-optimized code when developing, and it is also easier to understand (plus python linters, type checkers,etc can help you).
  2. You can specify this in memory views like int[::1, :] or int[:, ::1], right? Then yes you can do it.
  3. Yes:
import cython

int_or_float = cython.fused_type(int, float)

def func(numeric_array: int_or_float[:]):
    ...

I am not sure that Cython is pushing this very much? Where did you find this statement? My main concern is the points 2 and 3, which I don't think can be annotated as it is.

If you go through the 3.0 milestone (https://github.com/cython/cython/milestone/58), there are lots of issues/PRs that mention the pure python mode. Specially, in https://github.com/cython/cython/issues/4187, one of the bullet points is "present the Python syntax variant tab before (left of) the Cython syntax, as the first will eventually become preferable".

If you want to push this, consider making an example of _matrix_phase.pyx which isn't too long, but has some of the required things. ;)

Ok, will do!

pfebrer avatar Jun 15 '22 21:06 pfebrer

On the other hand, this code is working and I think there are other issues that are more pressing, rather than refactoring code. ;)

zerothi avatar Jun 16 '22 07:06 zerothi

Yes, it was more of a: "Can we do pure python for the new code?"

pfebrer avatar Jun 16 '22 07:06 pfebrer

Yes, it was more of a: "Can we do pure python for the new code?"

Ok, if the code performs as pure cython I see no obstacle ;)

zerothi avatar Jun 16 '22 07:06 zerothi

Good! Whenever you can, could you push your changes to the neighbours branch so that I can change the syntax there? :)

pfebrer avatar Jun 16 '22 10:06 pfebrer

I simply don't have the time right now to finish what I started there. Hopefully in a month or two... :(

zerothi avatar Jun 16 '22 19:06 zerothi

Ok, I thought you had finished but unpushed things, no worries!

pfebrer avatar Jun 17 '22 08:06 pfebrer

This is already on-going, we can close this as agreed and PR's are most welcome!

zerothi avatar Feb 21 '24 22:02 zerothi