momepy icon indicating copy to clipboard operation
momepy copied to clipboard

POC: function-based API

Open martinfleis opened this issue 3 years ago • 2 comments

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:

  1. 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?
  2. not sure if the current behaviour of the warning is not too noisy. It will show up every time you use class based API.
  3. 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).
  4. In most cases, W now depends on a unique_id column, 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, passing unique_id is 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.

martinfleis avatar Sep 13 '22 13:09 martinfleis

tl;dr – I like it.

  1. IMHO temporary code duplication is acceptable; just as long as we prune it down as soon as possible.
  2. 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 global that can be switched on.
  3. Very much on board for this.
  4. Also on board for this.

jGaboardi avatar Sep 14 '22 01:09 jGaboardi

I am going to revise this and, on top of the points listed above, build the new implementation on top of Graph.

martinfleis avatar Jan 08 '24 18:01 martinfleis

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

Impacted file tree graph

@@           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:

codecov[bot] avatar Jun 03 '24 13:06 codecov[bot]

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.

martinfleis avatar Jun 03 '24 13:06 martinfleis