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

shape_index is the same as radii_ratio #248

Open
martinfleis opened this issue Jun 9, 2023 · 3 comments
Open

shape_index is the same as radii_ratio #248

martinfleis opened this issue Jun 9, 2023 · 3 comments

Comments

@martinfleis
Copy link
Member

We have a duplication in shape indices. shape_index is the same as radii_ratio.

return numpy.sqrt(shapely.area(ga) / numpy.pi) / shapely.minimum_bounding_radius(ga)

and

esda/esda/shape.py

Lines 193 to 195 in 17c5f00

r_eac = numpy.sqrt(shapely.area(ga) / numpy.pi)
r_mbc = shapely.minimum_bounding_radius(ga)
return r_eac / r_mbc

are the same formulas.

We should probably deprecate one in favour of the other.

@ljwolf
Copy link
Member

ljwolf commented Jun 9, 2023

I like the “radii_ratio” name, since it expresses its structure as the ratio of two ideal shape radii. The two implementations are basically the same, though I find the one with more lines to be easier to understand.

We can always alias one as the other?

@martinfleis
Copy link
Member Author

Yeah, I also like the radii ratio better as shape index is too generic. All of them are shape indices. Agree that the multiline version is a bit more readable.

We can always alias one as the other?

Yeah but I don't think there's a ton of downstream usage of this so I would simply deprecate shape_index and add a note to radii_ratio docstring saying that it is also known as Schumm's shape index.

@ljwolf
Copy link
Member

ljwolf commented Jun 9, 2023

I will never pass up an opportunity to deprecate!

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

2 participants