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

feat: Add typehints to pyhf.tensor #1940

Merged
merged 23 commits into from
Aug 26, 2022
Merged

Conversation

kratsg
Copy link
Contributor

@kratsg kratsg commented Aug 12, 2022

Pull Request Description

See #1284.

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* Add typehints to the NumPy backend.
* Add .coveragerc file with configuration to ignore pyhf/typing.py.
  - ref: https://coverage.readthedocs.io/en/6.4.3/excluding.html#advanced-exclusion
  - Reverts part of PR #1942.
* Ignore pyhf/typing.py in Codecov.

@kratsg kratsg added docs Documentation related feat/enhancement New feature or request labels Aug 12, 2022
@kratsg kratsg self-assigned this Aug 12, 2022
@matthewfeickert matthewfeickert marked this pull request as draft August 12, 2022 06:55
@codecov
Copy link

codecov bot commented Aug 12, 2022

Codecov Report

Merging #1940 (1f66041) into master (f01dcf5) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1940      +/-   ##
==========================================
- Coverage   98.26%   98.24%   -0.03%     
==========================================
  Files          69       68       -1     
  Lines        4443     4380      -63     
  Branches      748      728      -20     
==========================================
- Hits         4366     4303      -63     
  Misses         45       45              
  Partials       32       32              
Flag Coverage Δ
contrib 26.55% <68.67%> (+0.51%) ⬆️
doctest 60.54% <97.59%> (-0.52%) ⬇️
unittests-3.10 96.14% <100.00%> (-0.01%) ⬇️
unittests-3.7 96.12% <100.00%> (-0.01%) ⬇️
unittests-3.8 96.16% <100.00%> (-0.01%) ⬇️
unittests-3.9 96.18% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pyhf/readxml.py 96.37% <ø> (ø)
src/pyhf/tensor/numpy_backend.py 98.58% <100.00%> (+0.06%) ⬆️
src/pyhf/typing.py

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@matthewfeickert matthewfeickert added the type checking Related to types and type checking label Aug 12, 2022
@kratsg kratsg force-pushed the feat/3rdpartyTyping branch 2 times, most recently from b8a7eab to 42edc96 Compare August 16, 2022 20:51
@kratsg
Copy link
Contributor Author

kratsg commented Aug 23, 2022

This adds back in #1937.

@kratsg kratsg marked this pull request as ready for review August 23, 2022 19:45
@kratsg kratsg temporarily deployed to github-pages August 26, 2022 04:15 Inactive
Copy link
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

Thanks for all this great work @kratsg. This is going to help a lot to move towards typing. 🙂

As things are passing this overall does look good. 👍 I just have a few questions to help me better understand what is happening as well.

I've also made some suggestions of changes, but most of them are just trying to take advantage of the import order changing or imports being added to apply isort here now. Feel free to ignore these of course, as they are only to try to reduce diff noise in the future.

.pre-commit-config.yaml Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
src/pyhf/tensor/numpy_backend.py Outdated Show resolved Hide resolved
src/pyhf/tensor/numpy_backend.py Outdated Show resolved Hide resolved
src/pyhf/tensor/numpy_backend.py Outdated Show resolved Hide resolved
src/pyhf/tensor/numpy_backend.py Show resolved Hide resolved
src/pyhf/typing.py Outdated Show resolved Hide resolved
src/pyhf/typing.py Outdated Show resolved Hide resolved
src/pyhf/typing.py Outdated Show resolved Hide resolved
@matthewfeickert matthewfeickert temporarily deployed to github-pages August 26, 2022 07:05 Inactive
@matthewfeickert matthewfeickert temporarily deployed to github-pages August 26, 2022 07:11 Inactive
Co-authored-by: Matthew Feickert <[email protected]>
@kratsg kratsg temporarily deployed to github-pages August 26, 2022 11:12 Inactive
@matthewfeickert matthewfeickert temporarily deployed to github-pages August 26, 2022 15:11 Inactive
Copy link
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

If we can get a Python version number to know when to update things, then this is all good IMO.

src/pyhf/tensor/numpy_backend.py Outdated Show resolved Hide resolved
@kratsg kratsg temporarily deployed to github-pages August 26, 2022 15:32 Inactive
Copy link
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

Awesome. Thank you @kratsg! 🚀

@matthewfeickert
Copy link
Member

Henry we're going to merge this, but if later you find something that you think we should revise please let us know!

@matthewfeickert matthewfeickert merged commit 3ac885e into master Aug 26, 2022
@matthewfeickert matthewfeickert deleted the feat/3rdpartyTyping branch August 26, 2022 15:56
kratsg pushed a commit that referenced this pull request Aug 26, 2022
* Update lower bound of the supported typing-extensions versions to v3.7.4.3
  to ensure SupportsIndex is available, which is used as of PR #1940.
   - Amends PR #1940.
* Update tests/constraints.txt to use typing-extensions==3.7.4.3.
matthewfeickert added a commit that referenced this pull request Aug 27, 2022
* Update lower bound of the supported tensorflow versions to v2.7.0 to ensure
  numpy v1.20.0+ is available.
   - numpy.typing is needed for PR #1940 and was added in numpy v1.20.0.
   - tensorflow v2.6.5 restricts numpy to 'numpy~=1.19.2' and tensorflow v2.7.0
     relaxes numpy constraints to 'numpy>=1.14.5'.
* Update lower bound of the supported jaxlib versions to v0.1.61.
   - jaxlib v0.1.60 required 'numpy>=1.12,<1.20' and jaxlib v0.1.61 relaxes this
     to 'numpy>=1.16'.
* Update tests/constraints.txt to use tensorflow==2.7.0 and jaxlib==0.1.61.
matthewfeickert pushed a commit that referenced this pull request Sep 7, 2022
* Add type hints to the tensor manager.
* Add type hints to some of the events management code for the decorators used.
* Expand the TensorBackend protocol and fix up the NumPy backend type hints to align properly.
   - Amends PR #1940.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related feat/enhancement New feature or request type checking Related to types and type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants