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

ENH: explore method for graph #617

Merged
merged 17 commits into from
Nov 3, 2023
Merged

ENH: explore method for graph #617

merged 17 commits into from
Nov 3, 2023

Conversation

knaaptime
Copy link
Member

@knaaptime knaaptime commented Oct 31, 2023

Screenshot 2023-10-31 at 12 22 10 AM

(the graph is actually based on network distance so that water observation is a known snapping issue :P)

@codecov
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Merging #617 (ce54246) into main (3976997) will increase coverage by 0.1%.
The diff coverage is 96.3%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #617     +/-   ##
=======================================
+ Coverage   84.4%   84.5%   +0.1%     
=======================================
  Files        139     139             
  Lines      14857   14963    +106     
=======================================
+ Hits       12536   12638    +102     
- Misses      2321    2325      +4     
Files Coverage Δ
libpysal/graph/base.py 97.6% <100.0%> (+<0.1%) ⬆️
libpysal/graph/tests/test_plotting.py 100.0% <100.0%> (ø)
libpysal/graph/tests/test_utils.py 98.8% <100.0%> (+0.1%) ⬆️
libpysal/graph/_plotting.py 92.9% <89.5%> (-2.8%) ⬇️

@martinfleis
Copy link
Member

Shall we aim for API parity with static plots here?

@martinfleis
Copy link
Member

@knaaptime a side note as I noticed the code in the screenshot above - for explore, CRS makes no difference. Map is always EPSG:3857 and is reprojected under the hood.

@knaaptime
Copy link
Member Author

@knaaptime a side note as I noticed the code in the screenshot above - for explore, CRS makes no difference. Map is always EPSG:3857 and is reprojected under the hood.

Ah, good call. I get it mixed up with contextily where I need to set it

@knaaptime
Copy link
Member Author

Shall we aim for API parity with static plots here?

Not sure where folks wanna take the api. Don’t have much of an opinion but figured this is enough to get us started

libpysal/graph/_plotting.py Outdated Show resolved Hide resolved
libpysal/graph/_plotting.py Outdated Show resolved Hide resolved
libpysal/graph/_plotting.py Outdated Show resolved Hide resolved
libpysal/graph/_plotting.py Outdated Show resolved Hide resolved
@knaaptime
Copy link
Member Author

Screenshot 2023-11-01 at 3 23 36 PM

@knaaptime
Copy link
Member Author

here's a simpler example. It's a touch verbose, i think that just is what it is. Should match the plot api now, i think

Screenshot 2023-11-01 at 4 38 52 PM

@martinfleis
Copy link
Member

This is great and apart from that one suggestion works as expected. Nice! Do you want to give a go to tests (ideally mirroring those from plot) or shall I do that?

@knaaptime
Copy link
Member Author

@martinfleis i'll take the first pass, but would probably be speediest if we tag team again. I'll try and do a thorough first draft. Would be interested to cut RCs for lib and esda today for kicking the tires

@knaaptime
Copy link
Member Author

knaaptime commented Nov 2, 2023

related, are there common patterns like this one that are becoming common enough to centralize in a testing module of lib? (a la numpy?). We now have explore in esda, pointpats, and here... its a tiny function so nbd, but there are probably other patterns used across the meta (and i know the original is from geopandas, i just mean there are probably pysal-common testing patterns we could stdize)

@martinfleis
Copy link
Member

The original is from folium :D. I think it makes sense to centralise stuff as assert_series_equal that does multiple things but utils like this can easily live in each. But I don't mind if we move it to libpysal.testing and re-use it across pysal

@knaaptime
Copy link
Member Author

looks like this will also require a minimum pin for a recent geopandas

@martinfleis
Copy link
Member

martinfleis commented Nov 3, 2023

We'll need to include a version of this as we currently plot both ij and ji. I will take back to this later today and expand tests a bit to ensure similar coverage we have in plot. I wanted to do it now but have family duties :).

# avoid plotting both ij and ji
edges = np.unique(np.sort(np.column_stack([codes]).T, axis=1), axis=0)

@martinfleis
Copy link
Member

Did a bit of refactor based on the code from plot but as a result we get the focal IDs for free so it simplifies the code a bit as well (no need to create that adj dataframe.

I think it is ready. It is smooooth!

Copy link
Member Author

@knaaptime knaaptime left a comment

Choose a reason for hiding this comment

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

sweet!

@knaaptime
Copy link
Member Author

i need one of you two to approve and merge though, :P

then lets do an RC?

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.

All looks great. One minor comment.

libpysal/graph/tests/test_plotting.py Show resolved Hide resolved
@martinfleis martinfleis merged commit 4290c11 into pysal:main Nov 3, 2023
10 checks passed
@martinfleis martinfleis changed the title [WIP] explore method for graph ENH: explore method for graph Nov 3, 2023
@martinfleis
Copy link
Member

then lets do an RC?

Do we need RC? Can we just do 4.9.0?

@knaaptime
Copy link
Member Author

we can just do 49 if you like

@jGaboardi
Copy link
Member

My opinion is that it's never a bad idea to do an RC just to make sure things are working properly. On the other hand, we can delete tags & releases if something is broken.

@martinfleis
Copy link
Member

I mean, what do you expect to test by RC in this case?

@knaaptime
Copy link
Member Author

the only thing with an "unnecessary" RC is that the changelog tooling techninically fails and you need to copy over the changelog

@jGaboardi
Copy link
Member

Making sure the actions (doc build & release) still work as they should. But I won't be stickler on that. Let's just try for the v4.9.0

@martinfleis
Copy link
Member

Hold on with that, there's a bug in this PR

@knaaptime
Copy link
Member Author

knaaptime commented Nov 3, 2023

aw crap lol i just pushed the tag

@knaaptime
Copy link
Member Author

looks like we're doing a 491 today 🙃

@martinfleis
Copy link
Member

it'll be 4.9.1 then :D

@jGaboardi
Copy link
Member

I done told yall! 🤣

@martinfleis
Copy link
Member

Sorry @jGaboardi, we should listen to you more :D

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