-
Notifications
You must be signed in to change notification settings - Fork 4
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 edm4eic::Tensor #96
Conversation
I had some comments on this but answered them myself while writing this out, I've added them here anyway in case it's useful but otherwise the type looks fine.
|
This is a good topic. My comment is that the current implementation in EICrecon PR does batch inference. As far as EDM is concerned, there is no difference between batch and single item evaluations, all tensors are supposed to have a concrete shape. ONNX model and the converters from/to-tensor are the one that have to know. Convertors obviously need to do the batching, and, like you say, folding unfolding, indeed is a possibility. For the second point, this is new to me, sounds like this is jagged tensor implementation. I'm not sure that ONNX models are coded like that, it actually supports "sequences" of tensors, which are more likely to be used, according to my intuition. Support for those may require another type in the EDM with a vector member for |
I have a demonstration model which (currently poorly) predicts the position and time of a hit from a varying sized collection of pixel hits. Here the model takes a tensorflow ragged tensor input and can be exported using the tf2onnx package: The model actually ends up taking two input tensors of different types. The first is just a float32 containing the input values and the second gives the position in the value array where different pseudo-clusters are separated as an int64. From what I see, I believe this would be completely compatible with the data type and onnx inference algorithm you currently have. |
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.
LGTM! I'm happy with this as is, but I wonder if might want to add additional element types down the line... For example, I can imagine having a bool (elementType==9
) might be useful for some ML classifiers.
I'd like to avoid the bool. It's probably useless (no existing exporter will use it) and might present an edge case when we add any kind of support for zero-copy. |
…icleIDs (#1618) ### Briefly, what does this PR introduce? <img width="582" alt="image" src="https://github.com/user-attachments/assets/a8c0aa47-e64f-4008-9702-444c6992ca3d"> This provides a calorimeter PID capability using ONNXruntime inference engine. The inference algorithm is generalized into a separate algorithm which inputs and outputs `edm4eic::Tensor` values introduced by eic/EDM4eic#96. A set of weights needed to run this are provided in eic/epic-data#22, and is generated using a script from eic/detector_benchmarks#91. ### What kind of change does this PR introduce? - [ ] Bug fix (issue #__) - [x] New feature (issue #__) - [ ] Documentation update - [ ] Other: __ ### Please check if this PR fulfills the following: - [ ] Tests for the changes have been added - [ ] Documentation has been added / updated - [ ] Changes have been communicated to collaborators ### Does this PR introduce breaking changes? What changes might users need to make to their code? No ### Does this PR change default behavior? Yes
…icleIDs (#1618) ### Briefly, what does this PR introduce? <img width="582" alt="image" src="https://github.com/user-attachments/assets/a8c0aa47-e64f-4008-9702-444c6992ca3d"> This provides a calorimeter PID capability using ONNXruntime inference engine. The inference algorithm is generalized into a separate algorithm which inputs and outputs `edm4eic::Tensor` values introduced by eic/EDM4eic#96. A set of weights needed to run this are provided in eic/epic-data#22, and is generated using a script from eic/detector_benchmarks#91. ### What kind of change does this PR introduce? - [ ] Bug fix (issue #__) - [x] New feature (issue #__) - [ ] Documentation update - [ ] Other: __ ### Please check if this PR fulfills the following: - [ ] Tests for the changes have been added - [ ] Documentation has been added / updated - [ ] Changes have been communicated to collaborators ### Does this PR introduce breaking changes? What changes might users need to make to their code? No ### Does this PR change default behavior? Yes
This adds a new type that facilitates exchange of tensor data for ML applications.