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: mean_interbuilding_distance and building_adjacency #556

Merged
merged 8 commits into from
Mar 10, 2024

Conversation

martinfleis
Copy link
Member

@martinfleis martinfleis commented Mar 8, 2024

Gosh I spent way too much time on this...

mean_interbuilding_distance:

35s new
57s old

Given my first attempt took 5mins, I call this a victory.

building_adjacency:

 old: 251 ms ± 14.6 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
 new: 422 ms ± 12.3 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

of which 330 ms is assign_self_weight which will be used in other functions so I'm wondering if it is worth adding a keyword self_weighted=True to skip this. In practice it is nearly negligible difference though...

These two functions also quite significantly change the API compared to the original. @jGaboardi can you give it a thought whether it makes sense in this way? I think it does and it is more flexible than before but it is different.

@martinfleis martinfleis added the enhancement New feature or request label Mar 8, 2024
@martinfleis martinfleis added this to the 0.8.0 milestone Mar 8, 2024
@martinfleis martinfleis self-assigned this Mar 8, 2024
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.6%. Comparing base (4037c70) to head (399cf39).
Report is 6 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #556     +/-   ##
=======================================
+ Coverage   97.4%   97.6%   +0.3%     
=======================================
  Files         26      30      +4     
  Lines       4328    4796    +468     
=======================================
+ Hits        4214    4682    +468     
  Misses       114     114             
Files Coverage Δ
momepy/functional/_distribution.py 100.0% <100.0%> (ø)
momepy/functional/tests/test_distribution.py 100.0% <100.0%> (ø)
momepy/functional/tests/test_shape.py 100.0% <100.0%> (ø)

@jGaboardi
Copy link
Member

These two functions also quite significantly change the API compared to the original. @jGaboardi can you give it a thought whether it makes sense in this way? I think it does and it is more flexible than before but it is different.

I think it makes sense, cleaner and more efficient for sure.

@martinfleis
Copy link
Member Author

of which 330 ms is assign_self_weight which will be used in other functions so I'm wondering if it is worth adding a keyword self_weighted=True to skip this. In practice it is nearly negligible difference though...

Solved in 0d897cc by cheaply (10ms) checking if the graph already has self-weights and assigning only if they do not. Which means that if you pass self-weighted graph, the new implementation takes about half the time compared to the original.

@jGaboardi jGaboardi merged commit 9f98387 into pysal:main Mar 10, 2024
13 checks passed
@martinfleis martinfleis deleted the blg_adj branch March 10, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants