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.

Sign up for GitHub

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

Jump to bottom

max val in rtree.silhouette_coeff #480

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

max val in rtree.silhouette_coeff #480

wants to merge 5 commits into from

Conversation

jGaboardi
Copy link
Member

This PR provides an overdo solution to a rough edge in cg.rtree.

xref pysal/spopt#57

@jGaboardi jGaboardi self-assigned this Oct 22, 2022
@jGaboardi jGaboardi added rough edge Something that's not a bug, but that gets in the way of "it just works." cg labels Oct 22, 2022
@jGaboardi jGaboardi requested review from ljwolf and sjsrey October 22, 2022 02:32
@jGaboardi
Copy link
Member Author

Tests are passing locally. Failure due to a fresh issue in geopandas through fiona.

@codecov
Copy link

codecov bot commented Oct 23, 2022 •
edited
Loading

Codecov Report

Merging #480 (94023dd) into main (8116db6) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #480     +/-   ##
=======================================
+ Coverage   79.9%   79.9%   +0.1%     
=======================================
  Files        122     122             
  Lines      13225   13225             
=======================================
+ Hits       10563   10572      +9     
+ Misses      2662    2653      -9
Impacted Files Coverage Δ
libpysal/cg/rtree.py 91.0% <100.0%> (ø)
libpysal/_version.py 40.7% <0.0%> (+2.7%) ⬆️

@martinfleis
Copy link
Member

Can we test this?

@jGaboardi
Copy link
Member Author

Can we test this?

Probably a good idea to... Let me try to carve out some time to look into it unless you already have an idea?

@martinfleis
Copy link
Member

unless you already have an idea

nope

@jGaboardi
Copy link
Member Author

In order to exactly test this we need to know of a situation where "there are corner cases when silhoutte for two clusters are exactly the same" (see here). I have pinged the OPs for a their use case, not sure how likely we are to hear back since the first post was nearly 5 years ago...

@jGaboardi jGaboardi requested a review from martinfleis October 26, 2022 17:05
@martinfleis
Copy link
Member

Tricky, yeah.

Out of curiosity, where is Rtree used in spopt? Can we use STRtree from shapely 2.0 or rtree package instead? Both are C++, so likely much faster.

@jGaboardi
Copy link
Member Author

jGaboardi commented Oct 26, 2022 •
edited
Loading

Out of curiosity, where is Rtree used in spopt?

Seems as it is actually not even being used in spopt at all. Suppose I can simply close out that issue there and leave this one open.

@martinfleis
Copy link
Member

Is it used anywhere? Is there a value in keeping it around or is that something that could be deprecated?

@jGaboardi
Copy link
Member Author

Is it used anywhere? Is there a value in keeping it around or is that something that could be deprecated?

Good question...

@ljwolf
Copy link
Member

ljwolf commented Oct 26, 2022

I believe it's only used in the "locators" code in libpysal, which I also don't think is used. +1 for deprecation of that and depending code.

@martinfleis
Copy link
Member

I believe it's only used in the "locators" code in libpysal

I've seen that but that whole module seems to be deprecated and can be removed.

sjsrey and others added 3 commits February 26, 2023 21:23
@sjsrey @jGaboardi @martinfleis
TEST: 3.11 dev coverage (pysal#516)
ded03e3 
* TEST: add 3.11 and shapely_dev coverage

* TEST: fix environment-file list

* TEST: remove option numba

* Update ci/311_shapely_dev.yaml

Co-authored-by: Martin Fleischmann <[email protected]>

---------

Co-authored-by: James Gaboardi <[email protected]>
Co-authored-by: Martin Fleischmann <[email protected]>
@jGaboardi
merge main
31ac0c7
@jGaboardi
merge main
94023dd
@jGaboardi jGaboardi changed the base branch from master to main February 27, 2023 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljwolf ljwolf Awaiting requested review from ljwolf

@sjsrey sjsrey Awaiting requested review from sjsrey

@martinfleis martinfleis Awaiting requested review from martinfleis

At least 1 approving review is required to merge this pull request.

Assignees

@jGaboardi jGaboardi

Labels
cg rough edge Something that's not a bug, but that gets in the way of "it just works."
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jGaboardi @martinfleis @ljwolf @sjsrey