-
Notifications
You must be signed in to change notification settings - Fork 1
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
Reduce overhead in DataStream.submit_data()
.
#147
Conversation
Benchmarks: for 32x32 float32: Macbook Pro M2 (Python 3.9.18): Hicatdeuxbis (Python 3.7.6): |
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.
What's with the addition of all the np.abs()
in the tests?
Nvm, your commit message explains it. |
@ivalaginja That's to avoid a warning when casting to unsigned integers from data that is both positive and negative. I presume it was there already, so it's kinda code sneak. It's not critical to this PR. |
Running the same benchmarks like @ehpor (after remembering that I had to recompile catkit2 😬 ), for 32x32 float32 data: Macbook Pro M1 (Python 3.9.12) thd2-controller (Python 3.9.12) |
Since this is a low-level change, I want to do a full end-to-end test on HiCAT before merging. |
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.
Looks good to me.
Hold merging until Emiel gives green light from the HiCAT test.
5c080c2
to
2fa57b0
Compare
Confirmed working on hardware. |
The old implementation performed type checking using Numpy functions, so that required a call from C++ back to Python, letting Python perform dtype operations, converting the result to a Python string, converting that string to a C++ string, then doing a C++ string comparison. The new code performs a C++ string comparison for the type directly using the dtype of the buffer format.