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

Mix with use of cython? #51

Closed
jorisvandenbossche opened this issue Oct 6, 2019 · 11 comments
Closed

Mix with use of cython? #51

jorisvandenbossche opened this issue Oct 6, 2019 · 11 comments

Comments

@jorisvandenbossche
Copy link
Member

One thing that I was wondering: would we be fine with also writing some things in Cython instead of C (especially things that would not use the ufunc machinery but need to written more manually) ?

Some things (eg the shapely interaction I was doing in https://github.com/caspervdw/pygeos/pull/47) might be simpler to do in cython.
On the other hand, that creates more different ways to do things, making the library as whole more complex.

@caspervdw
Copy link
Member

I agree with you on both arguments. Cython code is for sure more easy to read and write, but it will make this library more complex to manage and to build.

So I'd rather stay away from it now. Maybe at some point there will be a good reason like: we don't have the time to rewrite a certain heap of cython code.

@jorisvandenbossche
Copy link
Member Author

Meanwhile, I now lean towards using cython as well.

When we add some more custom algorithms (that don't fit in either a ufunc, or are not STRtree related), I think often the benefit of coding those in C disappears. And the added complexity of also adding cython into the mix is relatively limited IMO (at least when it comes to building, it's of course a different syntax to write).

For example, things like the PR at #93 for getting coordinates or the proposal in #127 / #128 for getting parts / exploding, will IMO be more approachable when we can write them in cython instead of C (especially if we limit those to 1D arrays).

@jorisvandenbossche
Copy link
Member Author

Any more thoughts on this?

cc @brendan-ward

@brendan-ward
Copy link
Contributor

Agreed on lowering the barrier to add in new code that doesn't strictly need to be written in C.

I'm open to adapting #130 to be in Cython instead. Should we try it there first, or is there a simpler bit of functionality where we could test out the approach?

I am a bit concerned about the additional complexities around packaging, but we can leverage some of the work already done in this space from fiona / rasterio.

@jorisvandenbossche
Copy link
Member Author

What additional complexities are you thinking of? (we already need to deal with building against and possibly including GEOS anyway)
It's of course not battle tested at all, but in #93, I didn't need to do that much to get cython set up in addition to our C code to build locally.

@brendan-ward
Copy link
Contributor

Sorry, I forgot about #93.

There might not be any additional complexities, other than including Cython as a build dependency (needs to be added to #93 to build on CI); the hardest parts are likely already solved by having a build environment for the C extensions...

What do you think about putting Cython files into a dedicated directory vs including alongside Python files (as in #93)? Also namespaces to keep the underlying . Personally, I like that lib is a dedicated namespace so that it is obvious we're calling into C at that point, and we might want to replicate some of that API separation with things implemented in Cython too.

@jorisvandenbossche
Copy link
Member Author

What do you think about putting Cython files into a dedicated directory vs including alongside Python files

Do you mean in a directory inside the python package? Or a top-level directory like src ?
A separate directory inside the python package is certainly fine.

@caspervdw
Copy link
Member

caspervdw commented Sep 20, 2020

I'd like to revive this ticket. I have the feeling that if we somehow 'set the stage' for cython contributions, we might address a few issues efficiently, while improving maintainability at the same time.

I however do not have any experience with cython worth mentioning. Do any of you feel up to proposing an architecture (setup.py / CI modifications / code structure) for starting this?

@brendan-ward
Copy link
Contributor

Apologies for not getting to a refactor of #130 using Cython sooner. That would be a good example to compare how the implementations and associated implementation difficulty differ between Cython and C approaches. I'll try to carve out some time for this in the next week.

@caspervdw
Copy link
Member

@brendan-ward Looking forward to it!

@jorisvandenbossche
Copy link
Member Author

Initial support for using cython was added in #197 (and in the meantime already used more)

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

No branches or pull requests

3 participants