momepy
momepy copied to clipboard
POC: function-based API
This is a proof-of-concept of the switch from the class-based API we have now to the functional API discussed in #340. It illustrates before-after as well as the transition period.
A couple of things to discuss:
- current classes allow users to retrieve all objects used in it, from the original gdf to weights or anything that has been created ad-hoc. We may want to retain such a behaviour but it leads to some code duplication in the meantime. Thoughts?
- not sure if the current behaviour of the warning is not too noisy. It will show up every time you use class based API.
- I would like to use this change to cleanup the arguments a bit as well, to get the naming consistency with the rest of PySAL (e.g.
spatial_weights -> w) and potentially even remove some stuff from momepy and move it elsewhere, like all our fancy ways of building lags (e.g. AverageCharacter). - In most cases, W now depends on a
unique_idcolumn, that has been used to index the weights. I would like to get rid of that and depend on the actual index of gdf instead, or at least have this controlled by a keyword. In most cases, passingunique_idis completely unnecessary. That all depends on the correct behaviour of libpysal in the first place, though (xref https://github.com/pysal/libpysal/pull/477). As a result, a lot would break but the final API in 1.0 will be much cleaner and easier to use.
@jGaboardi thoughts? It is a lot of breakage but if it happens all at once with a proper migration guide, it may not hurt that much. And since we're following semver here, it still acceptable.
tl;dr – I like it.
- IMHO temporary code duplication is acceptable; just as long as we prune it down as soon as possible.
- I think it is as noisy as it has to be to get people to switch to the functional API sooner than later. Maybe we add a manual setting for users that really don't want to make the change? Possibly keyword to skip the warning or maybe
globalthat can be switched on. - Very much on board for this.
- Also on board for this.
I am going to revise this and, on top of the points listed above, build the new implementation on top of Graph.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 97.9%. Comparing base (
4037c70) to head (1b54c16). Report is 25 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #406 +/- ##
=======================================
+ Coverage 97.4% 97.9% +0.5%
=======================================
Files 26 37 +11
Lines 4328 5444 +1116
=======================================
+ Hits 4214 5328 +1114
- Misses 114 116 +2
| Files | Coverage Δ | |
|---|---|---|
| momepy/dimension.py | 98.8% <100.0%> (+<0.1%) |
:arrow_up: |
| momepy/tests/test_dimension.py | 100.0% <100.0%> (ø) |
|
| momepy/utils.py | 98.9% <100.0%> (+0.1%) |
:arrow_up: |
I have updated this to reflect the current state of things. I'd like to merge it so we can use it as some classes are to be removed and adding a decorator would be nice to do now while we remember that.