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

API: deprecation decorators for transition to functional API #406

Merged
merged 8 commits into from
Jun 3, 2024

Conversation

martinfleis
Copy link
Member

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 API: Change the handling of IDs in from_dataframe constructors libpysal#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 martinfleis marked this pull request as draft September 13, 2022 13:43
@jGaboardi
Copy link
Member

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.

@martinfleis martinfleis mentioned this pull request Jan 12, 2023
@martinfleis
Copy link
Member Author

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

Copy link

codecov bot commented Jun 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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%) ⬆️
momepy/tests/test_dimension.py 100.0% <100.0%> (ø)
momepy/utils.py 98.9% <100.0%> (+0.1%) ⬆️

@martinfleis martinfleis changed the title POC: function-based API API: deprecation decorators for transition to functional API Jun 3, 2024
@martinfleis martinfleis marked this pull request as ready for review June 3, 2024 13:36
@martinfleis
Copy link
Member Author

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 martinfleis merged commit f7561e3 into pysal:main Jun 3, 2024
14 checks passed
@martinfleis martinfleis deleted the api_poc branch June 3, 2024 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants