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

Add type checks in constructors #210

Merged
merged 16 commits into from
Jul 9, 2022

Conversation

Saransh-cpp
Copy link
Member

@Saransh-cpp Saransh-cpp commented Jun 23, 2022

See #205

Added type checks in all the existing constructors.

@Saransh-cpp Saransh-cpp marked this pull request as draft June 23, 2022 09:17
@Saransh-cpp Saransh-cpp marked this pull request as ready for review June 24, 2022 09:20
@Saransh-cpp
Copy link
Member Author

The type checks work but I am not sure if the implementation is clean enough (especially in the awkward constructors).

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

See below.

src/vector/backends/awkward_constructors.py Outdated Show resolved Hide resolved
src/vector/backends/awkward_constructors.py Outdated Show resolved Hide resolved
src/vector/backends/awkward_constructors.py Outdated Show resolved Hide resolved
src/vector/backends/numpy.py Outdated Show resolved Hide resolved
src/vector/backends/numpy.py Outdated Show resolved Hide resolved
@Saransh-cpp
Copy link
Member Author

Thank you for such detailed explanations, @jpivarski! It took me some time to go through all the details, but they were extremely helpful! The code snippets were also very helpful!

Copy link
Member Author

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

The awkward constructors now look much better! The numpy type check still looks a bit messy as I encountered an edge case in the existing unit tests. Additionally, the type checks for numpy vectors is currently happening in the __array_finalise__ method but would it be better to perform the check in the __new__ method?

src/vector/backends/awkward_constructors.py Outdated Show resolved Hide resolved
src/vector/backends/numpy.py Outdated Show resolved Hide resolved
@Saransh-cpp
Copy link
Member Author

Oh, the light package does not include awkward as its dependency. I think I should use numpy dtypes for numpy vectors.

@codecov-commenter
Copy link

codecov-commenter commented Jun 25, 2022

Codecov Report

Merging #210 (2756495) into main (dfeead2) will increase coverage by 0.07%.
The diff coverage is 92.47%.

@@            Coverage Diff             @@
##             main     #210      +/-   ##
==========================================
+ Coverage   82.56%   82.63%   +0.07%     
==========================================
  Files          96       96              
  Lines       10475    10524      +49     
==========================================
+ Hits         8649     8697      +48     
- Misses       1826     1827       +1     
Impacted Files Coverage Δ
src/vector/backends/_numba_object.py 69.17% <ø> (ø)
src/vector/backends/awkward.py 75.89% <33.33%> (ø)
src/vector/backends/awkward_constructors.py 47.34% <90.00%> (+4.56%) ⬆️
src/vector/backends/numpy.py 78.08% <94.73%> (+0.57%) ⬆️
src/vector/_compute/lorentz/beta.py 100.00% <100.00%> (ø)
src/vector/_compute/lorentz/gamma.py 100.00% <100.00%> (ø)
src/vector/_compute/lorentz/unit.py 100.00% <100.00%> (ø)
src/vector/_compute/planar/unit.py 100.00% <100.00%> (ø)
src/vector/_compute/spatial/costheta.py 100.00% <100.00%> (ø)
src/vector/_compute/spatial/cottheta.py 100.00% <100.00%> (ø)
... and 6 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 da67139...2756495. Read the comment docs.

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

There are still a few instances of expensive data conversions for the sake of checking a data type.

src/vector/backends/numpy.py Outdated Show resolved Hide resolved
src/vector/backends/awkward_constructors.py Show resolved Hide resolved
src/vector/backends/awkward_constructors.py Outdated Show resolved Hide resolved
src/vector/backends/numpy.py Outdated Show resolved Hide resolved
src/vector/backends/object.py Outdated Show resolved Hide resolved
Saransh-cpp and others added 8 commits July 7, 2022 19:19
Co-authored-by: Jim Pivarski <jpivarski@users.noreply.github.com>
Co-authored-by: Jim Pivarski <jpivarski@users.noreply.github.com>
Co-authored-by: Jim Pivarski <jpivarski@users.noreply.github.com>
Copy link
Member Author

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Thank you for the review, @jpivarski! I have updated the PR with your suggestions! I have also updated the _is_type_safe function in backends/numpy.py to use the constructed vector. Now the function does not require converting a NumPy vector to a list and then to a NumPy array.

src/vector/backends/numpy.py Show resolved Hide resolved
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

This does it! The type-checks are fully general (no missing cases that I can see) and only metadata are iterated over, so I don't see any potential performance issues.

I'd say go ahead and merge it.

src/vector/backends/numpy.py Show resolved Hide resolved
@Saransh-cpp
Copy link
Member Author

Thank you for the constructive reviews! Merging this!

@Saransh-cpp Saransh-cpp merged commit 4e756fe into scikit-hep:main Jul 9, 2022
@Saransh-cpp Saransh-cpp deleted the saransh/add-type-checks branch July 9, 2022 07:57
@Saransh-cpp Saransh-cpp added this to the v0.9.0 milestone Feb 17, 2023
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.

3 participants