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

Add symmetric st-neighbors in local knox #121

Merged
merged 22 commits into from Sep 29, 2023
Merged

Add symmetric st-neighbors in local knox #121

merged 22 commits into from Sep 29, 2023

Conversation

sjsrey
Copy link
Member

@sjsrey sjsrey commented Sep 22, 2023

No description provided.

@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

Merging #121 (38b5c11) into main (a9d384c) will increase coverage by 5.3%.
Report is 4 commits behind head on main.
The diff coverage is 93.5%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #121     +/-   ##
=======================================
+ Coverage   64.1%   69.4%   +5.3%     
=======================================
  Files         12      12             
  Lines       1448    1769    +321     
=======================================
+ Hits         928    1228    +300     
- Misses       520     541     +21     
Files Coverage Δ
pointpats/spacetime.py 93.3% <93.5%> (+0.2%) ⬆️

... and 1 file with indirect coverage changes

@martinfleis
Copy link
Member

Could use your input on testing the new explore

@knaaptime, do you mean guidance on how to test the result of explore?

@knaaptime
Copy link
Member

Yeah. Looking at geopandas, like like you test for some known elements in the json output?

@martinfleis
Copy link
Member

Yes. Folium can give you a string of the HTML it generates, and we just try to ensure that those bits we need are there in the number of occurrences we expect them.

@sjsrey sjsrey changed the title Add symmetric st-neighbors in local knox [WIP - do not merge] Add symmetric st-neighbors in local knox Sep 26, 2023
@knaaptime
Copy link
Member

thinking about this just a little bit more, i wonder if we should set the default neighbor color to yellow or something instead of blue. If you're stuck in LISA mode, you might assume blue is some sort of coldspot

@martinfleis
Copy link
Member

That sounds very reasonable.

Copy link
Member

Choose a reason for hiding this comment

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

In cell 10, you can avoid apply.

sd_collisions["time_in_days"] = (sd_collisions.COLLISION_DATE - min_date).dt.days

Copy link
Member

Choose a reason for hiding this comment

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

useful, thanks

pointpats/spacetime.py Outdated Show resolved Hide resolved
pointpats/spacetime.py Outdated Show resolved Hide resolved
Comment on lines +1631 to +1635
if dataframe.crs is None:
warn(
"There is no CRS set on the dataframe. The KDTree will assume coordinates "
"are stored in Euclidean distances"
)
Copy link
Member

Choose a reason for hiding this comment

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

just fyi, GeoPandas does not warn in this case. We only warn when the crs is geographic. No CRS implies cartesian coordinates.

Copy link
Member

Choose a reason for hiding this comment

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

we hit this with the burkitt data which is stored in planar coords but doesnt have a CRS, so figured i'd add a warning just in case. I dont think we can always assume no CRS necessarily implies cartesian coords here.

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind the warning, just wanted to clarify the assumptions geopandas makes in case we want to do the same.

Copy link
Member

Choose a reason for hiding this comment

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

ah gotcha. I opted to warn here because the KDtree will still "work" (and thus the class will produce a result), but afaik there's no way to make it work correctly with cartesian coords, so its up to the user if they know what they're doing. Maybe it would be more conservative to fail instead?

"""Create an integer valued column for time"""
time_series = pandas.to_datetime(df[column], format=fmt)
min_time = time_series.min()
time_int = time_series.appy(lambda x: x - min_time)
Copy link
Member

Choose a reason for hiding this comment

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

Here's a typo in apply. I also don't think you need apply here.

Suggested change
time_int = time_series.appy(lambda x: x - min_time)
time_int = time_series - min_time

Copy link
Member

Choose a reason for hiding this comment

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

we were going to try an introspect any datetime col if passed, but then punted and decided the time col needs to be given as integer from the user.

so this is just a vestige of that idea and can be removed

return s_coords, t_coords


def _time_to_int(df, column, fmt="%Y%m%d"):
Copy link
Member

Choose a reason for hiding this comment

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

This is not used. Shall we remove it or plug it somewhere?

Copy link
Member

Choose a reason for hiding this comment

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

i think toss

knaaptime and others added 3 commits September 27, 2023 13:13
Co-authored-by: Martin Fleischmann <martin@martinfleischmann.net>
Co-authored-by: Martin Fleischmann <martin@martinfleischmann.net>
inference type in edge plots
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 looks okay to me, assuming that you have checked the correctness of the result (which I haven't).

@sjsrey sjsrey merged commit 8a96043 into pysal:main Sep 29, 2023
9 checks passed
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.

None yet

3 participants