-
Notifications
You must be signed in to change notification settings - Fork 89
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: to TensorFlow RaggedTensor #3210
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxymnaumchyk - good start! Please, run pre-commit run —all
locally before pushing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxymnaumchyk - Great job! The implementation is fine. However, the tests do not run in the CI because pytest
expects the test names starting with test_
. Please, check the suggestions.
Co-authored-by: Ianna Osborne <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxymnaumchyk - Great, thanks! All tests pass:
tests/test_3210_to_raggedtensor_from_raggedtensor.py::test_convert_to_raggedtensor PASSED [ 99%]
tests/test_3210_to_raggedtensor_from_raggedtensor.py::test_convert_from_raggedtensor PASSED [100%]
@jpivarski - if you don't have any objections I'll merge it. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good! It's well implemented and well tested. I made two suggestions.
Co-authored-by: Jim Pivarski <[email protected]>
@all-contributors please add @maxymnaumchyk for code |
I've put up a pull request to add @maxymnaumchyk! 🎉 |
Just a note: we'll need to get these new functions into the documentation. It needs to go into https://github.com/scikit-hep/awkward/blob/main/docs/reference/toctree.txt, as a new section
under the "Converting rectangular arrays" section and above the "Reading and writing files" section. That can be a quick little PR, and when the documentation-generating Action is done, there should be a "View Deployment" button that will let you see what the new documentation looks like. You should find the new documentation on the left side-bar of https://awkward-array.org/doc/main/reference/index.html . |
No description provided.