pysal / pointpats

Planar Point Pattern Analysis in PySAL
https://pysal.org/pointpats/
BSD 3-Clause "New" or "Revised" License
83 stars 26 forks source link

Ripley numpy #54

Closed ljwolf closed 4 years ago

ljwolf commented 4 years ago

migration of pysal/esda#115

ljwolf commented 4 years ago

OK, the notebook showing the translation from the old to new API is chugging along is chugging along.

I'd still like to

  1. document how do you specify tests using a given hull, either the types supported by default or a pre-sepcified hull polygon. This can be done by the end of the notebook, in the virginia example
  2. check p-values for validity. I need to double check the logic
  3. code a hand-computable example for the validity tests.
ljwolf commented 4 years ago

I've completed tests for all the helper functions, but still need to write the correctness tests.

I also need to document the hulls.

ljwolf commented 4 years ago

Tests are now in place and they're pretty comprehensive (if the backlog of commits is anything to show for it :)

I just need to document the hull logic in a notebook & then we're done.

ljwolf commented 4 years ago

I'm going through and re-working the distance_statistics notebook to show the examples using only the numpy-oriented code. We need to decide how to deprecate the existing distance statistics.

I'm torn on the simulator classes, since I don't implement cluster-poisson simulations yet.

@sjsrey @weikang9009 perspective on:

  1. how to deprecate the existing distance statistics? I'd like to just drop them entirely, but I know we should probably draw up a "migrating" document. I will not have the time to do this by July, and would like to get this into pointpats (or, send this up to scipy)?
  2. how to deal with the simulators? I'll have to investigate how to support cluster poisson processes, and I won't have the time to do this before release.
weikang9009 commented 4 years ago

I'm going through and re-working the distance_statistics notebook to show the examples using only the numpy-oriented code. We need to decide how to deprecate the existing distance statistics.

I'm torn on the simulator classes, since I don't implement cluster-poisson simulations yet.

@sjsrey @weikang9009 perspective on:

  1. how to deprecate the existing distance statistics? I'd like to just drop them entirely, but I know we should probably draw up a "migrating" document. I will not have the time to do this by July, and would like to get this into pointpats (or, send this up to scipy)?

I would suggest keeping the functions (but not contents) of existing distance statistics: replacing the implementation content with a DeprecationWarning and guiding the users to the new implementations for the next release. We may move to completely dropping these statistics later this year?

BTW, I do not see the unittests in this PR?

  1. how to deal with the simulators? I'll have to investigate how to support cluster poisson processes, and I won't have the time to do this before release.

Is it possible to reuse the code from process.py?

ljwolf commented 4 years ago

Thanks @weikang9009 for the perspective! I'm happy to deprecate them & remove their innards. But, this would also involve piping them into the new functions, which have substantially different APIs (e.g. not using any of the point pattern configuration/options arguments).

Could I deprecate the functions without piping them into the new code? I think this will be simpler, and provide a clean move from the old to the new code.

is it possible to resue the code from process.py? Yes, but I have not yet done that. We would have to allow for some way to allow for this in the simulate/simulate_from sampling loop. It could be possible if we defined an API like:

def simulator(hull, n_points : int): -> numpy.ndarray # of shape n_points,2

and then allowed the function to generate patterns. This would allow us to support the existing simulators, I think. What I have in there is fundamentally a bounding box rejection strategy.

ljwolf commented 4 years ago

Ah, this will force a test dependency on shapely?

ljwolf commented 4 years ago

OK, I've moved things around a bit in hoping to reuse the code from process.py as @weikang9009 mentions.

  1. I've split up the geometry operations & put them into [pointpats.geometry](). These are so that we can just use something like _area(shape) and get the area of a bounding box in numpy, a convex hull from scipy, a shapely polygon, or a pygeos geometry.
  2. I've moved the samplers into pointpats.random. This is so that you can do something like pointpats.random.poisson(hull) and get a poisson point pattern within hull. This is where I hoped to implement cluster_poisson(), but I'm having challenges getting _uniform_circle() to work while generating points within a hull.
ljwolf commented 4 years ago

Refactoring of simulators is done, plus two new simulators (pointpats.random.normal and pointpats.random.cluster_normal). Further, cluster_poisson now supports simulations from within an irregular envelope which, afaict, was not supported by pointpats.process.runif_in_circle.

ljwolf commented 4 years ago

Locally, this works for all the tests, and fails for some of the Knox statistics I didn't touch... not sure what's up there? We'll have to see in travis, but I think this is rtg. Documentation can be added later to beef up the info on the simulator alternatives.

While we should deprecate those (I think) I'm much less interested in doing so because they're performant.

ljwolf commented 4 years ago

OK, well, I'm not sure how to get the specification correct for travis, but tests pass locally aside from the Knox stuff (which I haven't touched). This is ready for review.

sjsrey commented 4 years ago

OK, well, I'm not sure how to get the specification correct for travis, but tests pass locally aside from the Knox stuff (which I haven't touched). This is ready for review.

See if this solution addresses the travis issues.

sjsrey commented 4 years ago

@ljwolf do you think this is the way we should go on the testing? Yesterday at the dev meeting we briefly mentioned this but I wanted to circle back in case it got lost in the flurry of activity.

If it is the way forward, please merge that pr into your branch, which will update this pr and i think have the tests pass.

ljwolf commented 4 years ago

Yes! Sorry, took the recommendation and merged it! I forgot about it when tinkering around today.