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: first bits of refactored dimension #532

Merged
merged 7 commits into from
Jan 9, 2024
Merged

Conversation

martinfleis
Copy link
Member

WIP but comes with a lot of simplification over the original API and quite nice speedups compared to original algos.

Using Prague as a benchmark:

longest_axis_length
1.59 s vs 3.76 s

perimeter_wall (including contiguity weights creation)
10.8 s vs 29.5 s

perimeter_wall (excluding contiguity weights creation)
4.91 s vs 21.2 s

The rest is using roughly the same implementation as before.

@jGaboardi the API change is a bit more radical than I originally anticipated but I believe we should do it properly this time.

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

codecov bot commented Jan 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2051f4b) 97.5% compared to head (288b4f9) 97.5%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #532   +/-   ##
=====================================
  Coverage   97.5%   97.5%           
=====================================
  Files         24      26    +2     
  Lines       4121    4187   +66     
=====================================
+ Hits        4018    4084   +66     
  Misses       103     103           
Files Coverage Δ
momepy/__init__.py 100.0% <100.0%> (ø)
momepy/functional/_dimension.py 100.0% <100.0%> (ø)
momepy/functional/tests/test_dimension.py 100.0% <100.0%> (ø)

@martinfleis martinfleis marked this pull request as ready for review January 9, 2024 14:27
@martinfleis
Copy link
Member Author

@jGaboardi I am not sure what is the best way of including the functional API in the documentation for the period when both are included. Let's not add it for now and figure it out before releasing? We'll need to rework the user guide as well so we can do that in one go.

@jGaboardi
Copy link
Member

Let's not add it for now and figure it out before releasing?

Works for me.

@jGaboardi
Copy link
Member

quite nice speedups compared to original algos.

Very nice!

@jGaboardi
Copy link
Member

jGaboardi commented Jan 9, 2024

This failure in dev is being fixed in #533, correct? Beat me to it!

@martinfleis
Copy link
Member Author

All green, we should be ready now.

momepy/functional/_dimension.py Outdated Show resolved Hide resolved
momepy/functional/_dimension.py Show resolved Hide resolved
Co-authored-by: James Gaboardi <jgaboardi@gmail.com>
@jGaboardi jGaboardi merged commit 4627ee2 into pysal:main Jan 9, 2024
11 checks passed
@martinfleis martinfleis deleted the perimeter branch January 9, 2024 15:35
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