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

adding alternative option in local_moran and moral_local_rate #205

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

adding alternative option in local_moran and moral_local_rate #205

wants to merge 9 commits into from

Conversation

tushar-corbic
Copy link

No description provided.

@sjsrey
Copy link
Member

sjsrey commented Apr 6, 2022

These look like useful changes. Thanks.

It would be helpful to add test coverage for new/changed functionality and update the documentation to surface these changes/enhancements.

esda/moran.py Outdated Show resolved Hide resolved
Copy link
Member

@martinfleis martinfleis left a comment

Choose a reason for hiding this comment

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

Can you also add tests as @sjsrey suggested above?

@sjsrey I'll leave a review of the actual formula to you, this a bit further away from my expertise and {spdep} doesn't seem to have one-tailed option to compare to (they have 'less', 'greater', 'two.sided').

esda/moran.py Outdated
@@ -509,9 +509,7 @@ def by_col(
def Moran_BV_matrix(variables, w, permutations=0, varnames=None):
"""
Bivariate Moran Matrix

Copy link
Member

Choose a reason for hiding this comment

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

It seems that your editor removed blank lines in the docstring. Can you please put them back (all of them)?

esda/moran.py Outdated
@@ -889,6 +875,9 @@ class Moran_Local(object):
value to use as a weight for the "fake" neighbor for every island. If numpy.nan,
will propagate to the final local statistic depending on the `stat_func`. If 0, then
the lag is always zero for islands.
alternative: string
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
alternative: string
alternative: {"two-tailed", "one-tailed"}
(default="two-tailed")

This follows the pattern used above.

esda/moran.py Outdated
Comment on lines 879 to 880
possible values -> "one-tailed"/ "two-tailed"
default value ->"two-tailed"
Copy link
Member

Choose a reason for hiding this comment

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

this part should contain a description of what the argument controls, i.e. what does it mean if I use "one-tailed" vs "two-tailed". Does not have to be detailed but just a hint, an explanation in plain words. Like in permutations, where we say "number of random permutations for calculation of pseudo p_values".

@tushar-corbic
Copy link
Author

@martinfleis @sjsrey , could you please tell me how to add test coverage for local_moran and moran_local_rate,
Thank you

@ljwolf
Copy link
Member

ljwolf commented Apr 12, 2022

Hey! this is looking very good, well done @tushar-corbic!

how to add test coverage

Sure! Normally, we add a method to the test class for that module. For Moran_Local, that's in esda/tests/test_moran.py. If you follow that pattern to ensure that the test works, and reproduces the same result every time, then that should be fine!

@tushar-corbic
Copy link
Author

Hey! this is looking very good, well done @tushar-corbic!

how to add test coverage

Sure! Normally, we add a method to the test class for that module. For Moran_Local, that's in esda/tests/test_moran.py. If you follow that pattern to ensure that the test works, and reproduces the same result every time, then that should be fine!

Thank you @ljwolf ,
I have added test coverage for local_moran.
Thank you

@tushar-corbic tushar-corbic closed this by deleting the head repository Mar 20, 2023
@martinfleis martinfleis reopened this Mar 20, 2023
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