Support for extracting volume patches #21759
Conversation
Summary of ChangesHello @danielenricocahall, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for extracting volume patches, which is a great addition for 3D data processing. The implementation is well-structured and follows the pattern of the existing extract_patches operation. The tests are also quite thorough.
I've found a few areas for improvement, mainly related to documentation accuracy, consistency in the implementation, and test correctness. Specifically, the docstring for the new function contains incorrect examples and descriptions, the Operation's call method has a minor inconsistency, and some of the new tests don't fully utilize their parameterization.
Please see my detailed comments below.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #21759 +/- ##
==========================================
- Coverage 82.68% 82.65% -0.03%
==========================================
Files 572 573 +1
Lines 58588 58958 +370
Branches 9161 9236 +75
==========================================
+ Hits 48442 48734 +292
- Misses 7819 7868 +49
- Partials 2327 2356 +29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
|
/gemini summary |
There was a problem hiding this comment.
Code Review
This pull request introduces extract_volume_patches for extracting patches from 3D volumes, which is a valuable addition. The implementation cleverly uses conv3d. However, I've identified some issues regarding clarity, correctness, and documentation. There's an inconsistency in the handling and documentation of the size parameter's dimensions, which could cause confusion and bugs. The variable names in the implementation are also misleading. Furthermore, a docstring example provides an incorrect output shape. I've also recommended enhancing the tests with a case for non-cubic patch sizes to improve robustness.
Summary of ChangesThis pull request introduces a new capability to Keras by adding Highlights
Changelog
Activity
|
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…' into add-volume-patching
|
/gemini summary |
|
/gemini review |
Summary of ChangesThis pull request introduces a new Highlights
Changelog
Activity
|
There was a problem hiding this comment.
Code Review
This pull request adds support for extracting patches from 3D volumes, analogous to the existing extract_patches for 2D images. The implementation is solid and follows the established pattern of using backend.nn.conv with an identity kernel. The tests are comprehensive, covering various scenarios including padding, strides, dilation, and data formats. I've identified a few areas for improvement, mainly in the docstrings for clarity and in the Operation class __init__ for more robust argument validation during symbolic execution.
|
/gemini summary |
Summary of ChangesThis pull request introduces a new Highlights
Changelog
Activity
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces extract_volume_patches, a 3D counterpart to the existing extract_patches function, which is a valuable addition for handling volumetric data. The implementation cleverly uses backend.nn.conv with an identity kernel for efficient patch extraction. The accompanying tests are thorough, covering various parameters like padding, strides, and data formats.
My review focuses on enhancing code clarity and maintainability. I've identified a couple of minor redundancies in the implementation of ExtractVolumePatches and suggested simplifications. Additionally, I've proposed changes to a few test cases to make them more explicit and easier to understand by ensuring the input data shape always aligns with the data_format being tested. Overall, this is a well-executed feature addition.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…' into add-volume-patching
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…' into add-volume-patching
|
Thanks for the PR!
Keeping them separate sounds fine to me. |
keras/src/ops/image.py
Outdated
|
|
||
|
|
||
| @keras_export("keras.ops.image.extract_volume_patches") | ||
| def extract_volume_patches( |
There was a problem hiding this comment.
For API consistency with our conv ops, we might want to name it extract_patches_3d or extract_3d_patches. "volume" could be any dimension beyond 2.
There was a problem hiding this comment.
Okay fixed! Went with extract_patches_3d to be closer to existing API
keras/src/ops/image.py
Outdated
| `(batch, depth, height, width, channels)`, while `"channels_first"` | ||
| corresponds to inputs with shape | ||
| `(batch, channels, depth, height, width)`. If not specified, | ||
| the value will default to `keras.config.image_data_format`. |
There was a problem hiding this comment.
It's a function, keras.config.image_data_format()
There was a problem hiding this comment.
Ah yes sorry, fixed
|
/gemini summary |
Summary of ChangesThis pull request introduces a new Highlights
Changelog
Activity
|
google-ml-butler
bot
added
kokoro:force-run
ready to pull
google-ml-butler
bot
removed
awaiting review
ready to pull
4 participants
Addresses #21624. Inspired by #21727, except made to be compatible with all backends
Notes
ExtractPatchesandExtractVolumePatchesOperations into a common base/mixin or is would that obfuscate?ExtractPatcheshave the same validation added to it's__init__as was suggested (and added) by Gemini inExtractVolumePatches?