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

Wrap legacy APIs #2702

Merged
merged 47 commits into from
Dec 18, 2023
Merged

Wrap legacy APIs #2702

merged 47 commits into from
Dec 18, 2023

Conversation

flying-sheep
Copy link
Member

@flying-sheep flying-sheep commented Oct 20, 2023

Part of scanpy 2.0

  • Closes #
  • Tests included or not required because:
  • Release notes not necessary because:

I implemented a Ruff check (PLR0917) for this, but setting max-positional-args = 3 would make this massive PR even larger, so I opted to do it in a separate one.

Reviewers

Your main job is to check if the position of the * makes sense for each exported function (i.e. the ones with the @legacy_api decorator). I tried my best to base it on internal usage of each API, but one placement or the other might be to early.

The only real logic changes are in scanpy/tests/test_package_structure.py. This PR:

  • makes public APIs with more than 5 parameters keyword-only without breaking backwards compatibility (for now)
  • makes private APIs with more than 5 parameters keyword-only
  • checks that we don’t internally use the old positional APIs using a warning filter with action='error'
  • checks that APIs use our convention for the copy parameter (adata as first param, returns adata type or None`)
  • manually checked that APIs use our convention filename: Path | str/path: Path | str

Follow-up changes

@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

Merging #2702 (8aec872) into master (86dc4d5) will increase coverage by 0.29%.
The diff coverage is 83.20%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2702      +/-   ##
==========================================
+ Coverage   72.53%   72.83%   +0.29%     
==========================================
  Files         111      111              
  Lines       12185    12378     +193     
==========================================
+ Hits         8838     9015     +177     
- Misses       3347     3363      +16     
Files Coverage Δ
scanpy/_compat.py 44.44% <100.00%> (+3.96%) ⬆️
scanpy/_settings.py 90.24% <100.00%> (+0.07%) ⬆️
scanpy/datasets/_datasets.py 86.04% <100.00%> (+0.21%) ⬆️
scanpy/experimental/pp/_highly_variable_genes.py 63.46% <ø> (ø)
scanpy/experimental/pp/_normalization.py 95.06% <100.00%> (ø)
scanpy/external/pp/_bbknn.py 58.33% <100.00%> (+8.33%) ⬆️
scanpy/external/pp/_dca.py 58.33% <100.00%> (+8.33%) ⬆️
scanpy/external/pp/_harmony_integrate.py 86.66% <100.00%> (+2.05%) ⬆️
scanpy/external/pp/_hashsolo.py 89.74% <100.00%> (+0.17%) ⬆️
scanpy/external/pp/_scanorama_integrate.py 88.88% <100.00%> (+0.88%) ⬆️
... and 58 more

@flying-sheep flying-sheep marked this pull request as ready for review December 3, 2023 12:40
@flying-sheep flying-sheep added this to the 1.10.0 milestone Dec 3, 2023
@flying-sheep flying-sheep self-assigned this Dec 5, 2023
Copy link
Contributor

@ilan-gold ilan-gold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking, but I am pretty worried about the breaking changes. This isn't really my call though.

scanpy/external/pp/_bbknn.py Outdated Show resolved Hide resolved
scanpy/external/pp/_hashsolo.py Show resolved Hide resolved
scanpy/external/tl/_phenograph.py Show resolved Hide resolved
scanpy/external/tl/_phenograph.py Outdated Show resolved Hide resolved
scanpy/neighbors/__init__.py Show resolved Hide resolved
scanpy/plotting/_anndata.py Outdated Show resolved Hide resolved
scanpy/preprocessing/_simple.py Show resolved Hide resolved
scanpy/tools/_utils_clustering.py Show resolved Hide resolved
scanpy/tests/test_package_structure.py Show resolved Hide resolved
scanpy/tests/test_package_structure.py Show resolved Hide resolved
@flying-sheep flying-sheep removed the request for review from ivirshup December 18, 2023 13:33
@flying-sheep flying-sheep merged commit 266c054 into master Dec 18, 2023
11 checks passed
@flying-sheep flying-sheep deleted the short-apis branch December 18, 2023 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants