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

Knox enhancements #111

Merged
merged 48 commits into from
Sep 22, 2023
Merged

Knox enhancements #111

merged 48 commits into from
Sep 22, 2023

Conversation

sjsrey
Copy link
Member

@sjsrey sjsrey commented Jul 20, 2023

Features

  • More efficient Global Knox
  • Analytical inference for Global Knox
  • Local Knox
  • Analytical inference for Local Knox
  • Conditional permutation inference for Local Knox
  • static plotting for significant clusters
  • interactive plotting for significant clusters

Todo

  • add tests
  • polish user notebook (add plotting)

@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Merging #111 (8ebd6a9) into main (416bca9) will increase coverage by 6.70%.
Report is 6 commits behind head on main.
The diff coverage is 78.40%.

@@            Coverage Diff             @@
##             main     #111      +/-   ##
==========================================
+ Coverage   59.57%   66.28%   +6.70%     
==========================================
  Files          10       12       +2     
  Lines        1405     1732     +327     
  Branches      246        0     -246     
==========================================
+ Hits          837     1148     +311     
- Misses        518      584      +66     
+ Partials       50        0      -50     
Files Changed Coverage Δ
pointpats/spacetime.py 83.10% <78.40%> (-6.27%) ⬇️

... and 9 files with indirect coverage changes

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.

This is nice! Could we try to get this and #105 in and cut a release with sklearn 1.3.0 compatibility fixes we did here?

pointpats/spacetime.py Outdated Show resolved Hide resolved
@sjsrey
Copy link
Member Author

sjsrey commented Aug 24, 2023

This is nice! Could we try to get this and #105 in and cut a release with sklearn 1.3.0 compatibility fixes we did here?

Yes. Perhaps during the sprint on 8-24?

@sjsrey sjsrey changed the title Knox enhancements (WIP) Knox enhancements Sep 3, 2023
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.

Looking good!

@knaaptime
Copy link
Member

@martinfleis not to distract you from working on lib at the moment, but a quick thing in case you have an idea..

we cooked up a little explore method on the localknox class that works pretty nicely. One killer thing would be to have an onclick or mouseover function that highlights neighbors when you interact with focal. I think the current method has all the info necessary to get there, but not sure how to go about rigging that up with folium. Curious if you know how

@martinfleis
Copy link
Member

Not possible afaik. The styling function sees only a single item of the json and I didn't figure out a way of highlighting from B based on an interaction with a geom A.

@knaaptime
Copy link
Member

bummer. pretty sure that's dead simple in vanilla leaflet. I was trying to send back IDs of neighbors as an attribute of focal to highlight those vals in a different color or something

thanks for the insight

@martinfleis
Copy link
Member

I am not so sure, folium is pretty close in API to leaflet. But I suppose it would need a bit of custom JS which is easy when you have a leaflet map, not so much with folium.

Maybe bokeh or geoviews may offer solutions.

@sjsrey sjsrey merged commit 8ebd6a9 into pysal:main Sep 22, 2023
@martinfleis martinfleis mentioned this pull request Sep 25, 2023
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