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

Move points_in_polygon to Cython #250

Merged
merged 3 commits into from
May 26, 2017
Merged

Conversation

jwiggins
Copy link
Member

@jwiggins jwiggins commented Nov 5, 2016

Since I'm maneuvering to eliminate the agg extension from kiva, I need to do something with points_in_polygon. I believe the only reason it was part of that extension was to cut down on the number of extension modules being built. Now it lives in its own extension.

NOTE: I don't want to merge this until after 4.6.0 is released.

This function was part of the agg C-extension. It has no dependence on anything
in AGG, so it can be moved into its own extension.
@codecov-io
Copy link

codecov-io commented Nov 5, 2016

Current coverage is 52.08% (diff: 100%)

Merging #250 into master will increase coverage by 0.02%

@@             master       #250   diff @@
==========================================
  Files           126        127     +1   
  Lines         13176      13180     +4   
  Methods           0          0          
  Messages          0          0          
  Branches       1943       1941     -2   
==========================================
+ Hits           6859       6865     +6   
+ Misses         5872       5870     -2   
  Partials        445        445          

Powered by Codecov. Last update 5e4e7f1...b170a17

@corranwebster
Copy link
Contributor

Generally a good idea, although I haven't tried this out.

@corranwebster
Copy link
Contributor

This is a backwards-incompatible change which will require updating some stuff in Chaco (and at least one other internal project I know of).

Possibly this should be kept in kiva (although not in kiva.agg), rather than moved to enable? I'm in two minds.

@jwiggins
Copy link
Member Author

jwiggins commented Dec 2, 2016

I'm planning for kiva.agg to go away. If the import has to change, why not move it into enable at the same time? It doesn't seem drawing related, so that's my argument for not leaving it in kiva.

@corranwebster corranwebster added this to the 5.0.0 Release milestone Dec 19, 2016
@corranwebster
Copy link
Contributor

While this is currently used for hit detection and point selection by Chaco, this is the sort of routine that can be useful for higher-level drawing routine optimization (eg. avoiding drawing points which are obscured).

In other words, although it's not about drawing, it is about pure geometry, which seems a more Kiva-y thing than an Enable-y thing.

@jwiggins
Copy link
Member Author

Agreed. I've moved the code back to kiva, but still outside of agg.

@corranwebster corranwebster merged commit e09da89 into master May 26, 2017
@corranwebster corranwebster deleted the refactor/hit-testing branch May 26, 2017 10:27
@jwiggins
Copy link
Member Author

Thanks for reviewing!

@rkern
Copy link
Member

rkern commented Jun 26, 2017

@jwiggins Can you please expose it back in kiva.agg with a deprecation warning? This is part of the public API and used in Chaco, so now all of the Chaco PR Travis runs are failing. Migrating Chaco isn't hard, but then Chaco would be relying on an unreleased version of Enable. The deprecation period makes the coordination easier.

@jwiggins
Copy link
Member Author

Sure thing

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

Successfully merging this pull request may close these issues.

4 participants