Test update test#13
Closed
jholveck wants to merge 16 commits into
Closed
Conversation
Currently, this shares memory with the .raw bytearray in some cases. We should consider whether we want to change that to always copy, or just wait until we have the full shared buffer support. This implementation always does the array manipulation on the CPU. For PyTorch or TensorFlow, the user is likely to want to do the array manipulation on the GPU for performance. This is very easy to implement.
Python 3.9 doesn't define TypeAlias, and it's not required in this case, so we can just remove it.
The reverted change was from a temporary test, but I accidentally included it in the commit. It's not relevant to the issue at hand.
The docstrings weren't all in Sphinx format, and could use a little clarification in places. TensorFlow itself allows NumPy dtypes and DataType enums, so expand to_tensorflow to accept those too.
The main point of this commit is to be explicit about the memory-sharing semantics: mostly, that we don't guarantee memory sharing one way or another. More to the point, that if they modify the pixel data in one of the exported objects, it may or may not affect the other exported objects. Memory sharing isn't expected to affect most users, since they are unlikely to access the pixel data with two different exported objects, but it's best to be explicit. To have a good place to put the memory-sharing discussion, I added a new section to the usage.rst file, listing the ways to access the pixel data. This is also where I documented the NumPy array interface protocol, which previously didn't show up in our Sphinx docs. (It might have been in the docs for 10.1, which didn't use autodoc.) I also put a discussion of the alpha channel in this new section: one thing I keep forgetting is to delete the alpha channel when using Matplotlib, and wondering why it's blank. TODO: We probably should document `grab` in the usage.rst file for the new section to make sense. In the process, I've also added intersphinx links to libraries like PIL and NumPy, so that our Sphinx docs will link to them. While checking that, I fixed a few other references (like referring to bytes with :py:type:, when it's indexed under :py:class:).
Doc fixes. Speed up converting to a NumPy array in RGB order. That sort of thing.
While I was writing this, I realized something else. We may want to change the to_torch and to_tensorflow methods to accept device and stream arguments, and do the channel and layout shuffling on the GPU instead of the CPU. This wouldn't require us to natively support CUDA or anything like that; we'd just be using PyTorch / TensorFlow's existing CUDA support, which is easy. This demo still (as before) shows the GPU-focused way to do the channel and layout shuffling, presented as an alternative to using to_torch. A quick test shows about 107 ms / frame (to_torch) vs. about 62 ms / frame (GPU-focused routine), and that's including the CNN itself too. So there's a very significant benefit there. (That's not quite a fair comparison, since the GPU-focused routine is exactly what this program needs, but not so it'd be a big difference.)
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
While improving performance earlier in this PR, I had introduced a case where to_torch would fail only for the RGB order. It also seemed likely that some cases might fail only if using particular combinations of channel order and layout, so we just test them all.
jholveck
force-pushed
the
test-update-test
branch
from
June 28, 2026 01:07
20eecfd to
c3872f3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes proposed in this PR
Fixes #
(...)
./check.shpassed