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

add Lee (2001) local spatial Pearson #51

Merged
merged 14 commits into from
Jul 1, 2019
Merged

add Lee (2001) local spatial Pearson #51

merged 14 commits into from
Jul 1, 2019

Conversation

ljwolf
Copy link
Member

@ljwolf ljwolf commented Mar 4, 2019

This adds the Lee local spatial pearson estimators, alongside a wrapped version of the R code you can use to get them from spdep. This still needs

  • unittests
  • docstrings
  • remove the R code or hide it
  • remove/swap out sklearn format & array checking.

@ljwolf
Copy link
Member Author

ljwolf commented Mar 4, 2019

also maybe want to keep/return the "spatial smoothing scalars" from lee

Copy link
Member

@jGaboardi jGaboardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with minor changes/updates.

class Spatial_Pearson(BaseEstimator):
def __init__(self, connectivity=None, permutations=999):
self.connectivity = connectivity
self.permutations = permutations
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be type caster here just in case the user give permutations as something other than an integer?

self.permutations = int(permutations)

esda/lee.py Outdated Show resolved Hide resolved
class Local_Spatial_Pearson(BaseEstimator):
def __init__(self, connectivity = None, permutations=999):
self.connectivity = connectivity
self.permutations = permutations
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment in line 39

esda/lee.py Show resolved Hide resolved
esda/lee.py Outdated Show resolved Hide resolved
esda/lee.py Outdated Show resolved Hide resolved
esda/lee.py Outdated Show resolved Hide resolved
esda/lee.py Outdated Show resolved Hide resolved
esda/lee.py Show resolved Hide resolved
esda/lee.py Outdated Show resolved Hide resolved
@sjsrey sjsrey added the WIP work in progress (for discussion) label Mar 4, 2019
@ljwolf
Copy link
Member Author

ljwolf commented Mar 4, 2019

The significance testing is not correct; not sure how I'm screwing up the form of the null statistic, but it's definitely not right. Will look into.

@ljwolf
Copy link
Member Author

ljwolf commented Mar 5, 2019

I think I've fixed up that code for the local inference. Stil need docstrings & unittests.

Before I do that, can I get a ruling on the sklearn dependency from maintainers? If I rewrite this, I'd use array checking logic from spreg, or I'd write my own.

@ljwolf
Copy link
Member Author

ljwolf commented Mar 5, 2019

Oh also on the API; this uses init().fit()-style stuff, which is different from the existing pattern.

@jGaboardi
Copy link
Member

@ljwolf Are there any real drawbacks to having scikit-learn as a dependency besides simply having another dependency? Since scikit-learn is included in every major Anaconda distribution, this seems to me to be a reasonable way forward.

@ljwolf ljwolf mentioned this pull request Jun 24, 2019
8 tasks
@sjsrey
Copy link
Member

sjsrey commented Jun 26, 2019

I think I've fixed up that code for the local inference. Stil need docstrings & unittests.

Before I do that, can I get a ruling on the sklearn dependency from maintainers? If I rewrite this, I'd use array checking logic from spreg, or I'd write my own.

I think sklearn dependency is +1

@ljwolf
Copy link
Member Author

ljwolf commented Jun 27, 2019

docstrings and tests are added.

@sjsrey
Copy link
Member

sjsrey commented Jun 29, 2019

@ljwolf is this WIP stil, or ready to merge?

@ljwolf
Copy link
Member Author

ljwolf commented Jul 1, 2019

It's ready if the sklearn dependency is accounted for?

@sjsrey sjsrey removed the WIP work in progress (for discussion) label Jul 1, 2019
@sjsrey sjsrey merged commit d7a75b3 into pysal:master Jul 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants