Skip to content

Use numpy.typing.DTypeLike #594

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

Closed
wants to merge 4 commits into from
Closed

Conversation

tomwhite
Copy link
Collaborator

@tomwhite tomwhite commented Jun 1, 2021

This fixes half of #452. NumPy's ArrayLike isn't a drop-in replacement for ours, since it represents something that can be converted to a NumPy array (including lists, for example), whereas we use it in the sense of "something that looks like an array" (which could be a Dask array, for example).

@tomwhite
Copy link
Collaborator Author

tomwhite commented Jun 7, 2021

The test is failing due to some interaction between numpy.typing and scanpydoc.

@tomwhite
Copy link
Collaborator Author

tomwhite commented Jun 8, 2021

The test is failing due to some interaction between numpy.typing and scanpydoc.

I think it is failing only on Python 3.7, since SupportsIndex is not there (introduced in Python 3.8), and Python 3.7 when called from scanpydoc doesn't use typing-extensions.

One fix might be to require Python >=3.8 for the doc build, so we could skip "Check for Sphinx doc warnings" in build.yml when running Python 3.7. (Not sure how easy it is to do that though.)

@@ -31,6 +31,7 @@ jobs:
- name: Run pre-commit
uses: pre-commit/action@v2.0.0
- name: Check for Sphinx doc warnings
if: contains(matrix.python-version, '3.8')
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ravwojdyla thanks for your (offline) suggestion to do this. The workflow syntax works perfectly. But the build is failing on 3.8 too, so I need to look into that!

@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2021

Codecov Report

Merging #594 (4be3bf1) into main (70a91ba) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #594   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           35        35           
  Lines         2749      2762   +13     
=========================================
+ Hits          2749      2762   +13     
Impacted Files Coverage Δ
sgkit/io/bgen/bgen_reader.py 100.00% <100.00%> (ø)
sgkit/io/utils.py 100.00% <100.00%> (ø)
sgkit/io/vcf/vcf_reader.py 100.00% <100.00%> (ø)
sgkit/stats/ld.py 100.00% <100.00%> (ø)
sgkit/stats/pca.py 100.00% <100.00%> (ø)
sgkit/typing.py 100.00% <100.00%> (ø)
sgkit/utils.py 100.00% <100.00%> (ø)
sgkit/window.py 100.00% <100.00%> (ø)
sgkit/variables.py 100.00% <0.00%> (ø)
sgkit/stats/popgen.py 100.00% <0.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70a91ba...4be3bf1. Read the comment docs.

@tomwhite
Copy link
Collaborator Author

Various extra changes were needed to get Sphinx to build the docs. The type annotation is very off-putting for users though:

dtypes

I was expecting the annotation to be DTypeLike, linked to https://numpy.org/devdocs/reference/typing.html#numpy.typing.DTypeLike.

This problem was discussed in Pandas in pandas-dev/pandas#33025. It might be worth investigating further to see if we can improve this.

@ravwojdyla
Copy link
Collaborator

@tomwhite nice, should we merge this and investigate/improve in a separate PR/issue?

@tomwhite
Copy link
Collaborator Author

@ravwojdyla I'd rather wait and see if there's a way to avoid the off-putting type signatures in docs, as I don' think this is an improvement as it stands.

@mergify
Copy link
Contributor

mergify bot commented Jul 8, 2021

This PR has conflicts, @tomwhite please rebase and push updated version 🙏

@mergify mergify bot added the conflict PR conflict label Jul 8, 2021
@tomwhite tomwhite closed this Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflict PR conflict
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants