Conversation
- Removes hardcoded dtype exclusion list from pad_nd() - Enables torch.nn.functional.pad for bool and integer dtypes - Try-except fallback to numpy still handles unsupported combinations - Fixes Project-MONAI#7842
- Checks if tensorboard is installed before attempting to create SummaryWriter - Provides installation instructions when tensorboard is missing - Improves user experience for debugging missing dependencies - Addresses issue Project-MONAI#7980
📝 Walkthroughmonai/apps/reconstruction/transforms/dictionary.pyWalkthroughAdds an Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes monai/handlers/tensorboard_handlers.pyWalkthroughWhen no Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes monai/transforms/croppad/functional.pyWalkthroughSimplifies Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/apps/reconstruction/transforms/dictionary.py (1)
54-77:⚠️ Potential issue | 🟡 Minor
__call__docstring doesn't document theimage_onlyskip behaviour.When
image_only=Trueand aMetaTensoris found, the key is silently omitted from the returned dict. That's a non-obvious contract change that downstream callers need to know about. TheReturnssection should note it.📝 Suggested docstring addition
Returns: - the new data dictionary + the new data dictionary. Note: when ``image_only=True``, + keys whose meta values are MetaTensor instances are skipped + and will not appear in the returned dict.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/apps/reconstruction/transforms/dictionary.py` around lines 54 - 77, The __call__ docstring for Dictionary transform omits the behavior when image_only=True where keys whose extracted value is a MetaTensor are silently skipped; update the docstring of __call__ to document that when image_only is True and an extracted value from d[self.meta_key] is a MetaTensor the transform will not add that key to the returned dict (i.e., the key is omitted), and clarify how this interacts with allow_missing_keys and the returned dictionary shape so callers know to expect missing keys in that case.
🧹 Nitpick comments (1)
monai/apps/reconstruction/transforms/dictionary.py (1)
49-49: Overly long__init__signature — wrap for readability.Line 49 exceeds PEP8's 79/99-char limit. As per coding guidelines, Python code should adhere to PEP8 style.
♻️ Suggested formatting
- def __init__(self, keys: KeysCollection, meta_key: str, allow_missing_keys: bool = False, image_only: bool = False) -> None: + def __init__( + self, + keys: KeysCollection, + meta_key: str, + allow_missing_keys: bool = False, + image_only: bool = False, + ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/apps/reconstruction/transforms/dictionary.py` at line 49, The __init__ method signature in the class is too long (exceeds PEP8 width); reformat the signature for readability by breaking the parameter list across multiple lines, e.g. place the opening parenthesis after __init__, then put each parameter (keys: KeysCollection, meta_key: str, allow_missing_keys: bool = False, image_only: bool = False) on its own indented line preserving type hints and defaults, and finish with the closing parenthesis and -> None on its own line (or keep the return annotation on the def line if preferred) so the signature fits within the style guide; update the definition of __init__ accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monai/apps/reconstruction/transforms/dictionary.py`:
- Around line 66-71: The code references MetaTensor in the dictionary extraction
logic (used in the block that checks self.image_only and calls
isinstance(extracted_value, MetaTensor) when populating d[key]) but MetaTensor
is not imported; fix by adding the proper import (e.g., add "from monai.data
import MetaTensor" alongside the existing monai imports) so that the isinstance
check in that extraction loop (the code around extracted_value =
d[self.meta_key][key] and the self.image_only branch) does not raise NameError
at runtime.
In `@monai/handlers/tensorboard_handlers.py`:
- Around line 53-57: Capture the boolean success flag returned by
optional_import when importing SummaryWriter (e.g., SummaryWriter,
summary_writer_available = optional_import(...)) and store that alongside the
SummaryWriter symbol; then in TensorBoardHandler.__init__ check the boolean flag
(summary_writer_available) instead of testing "SummaryWriter is None" and raise
the RuntimeError with the helpful install message if the flag is False so the
guard triggers before the OptionalImportError stub is invoked.
---
Outside diff comments:
In `@monai/apps/reconstruction/transforms/dictionary.py`:
- Around line 54-77: The __call__ docstring for Dictionary transform omits the
behavior when image_only=True where keys whose extracted value is a MetaTensor
are silently skipped; update the docstring of __call__ to document that when
image_only is True and an extracted value from d[self.meta_key] is a MetaTensor
the transform will not add that key to the returned dict (i.e., the key is
omitted), and clarify how this interacts with allow_missing_keys and the
returned dictionary shape so callers know to expect missing keys in that case.
---
Nitpick comments:
In `@monai/apps/reconstruction/transforms/dictionary.py`:
- Line 49: The __init__ method signature in the class is too long (exceeds PEP8
width); reformat the signature for readability by breaking the parameter list
across multiple lines, e.g. place the opening parenthesis after __init__, then
put each parameter (keys: KeysCollection, meta_key: str, allow_missing_keys:
bool = False, image_only: bool = False) on its own indented line preserving type
hints and defaults, and finish with the closing parenthesis and -> None on its
own line (or keep the return annotation on the def line if preferred) so the
signature fits within the style guide; update the definition of __init__
accordingly.
| extracted_value = d[self.meta_key][key] | ||
| # When image_only is True, skip if the extracted value is a MetaTensor | ||
| # to preserve metadata associations | ||
| if self.image_only and isinstance(extracted_value, MetaTensor): | ||
| continue | ||
| d[key] = extracted_value # type: ignore |
There was a problem hiding this comment.
MetaTensor is not imported — NameError at runtime.
MetaTensor is referenced at line 69 but never imported in this file. When image_only=True and a MetaTensor value is encountered, the code raises NameError: name 'MetaTensor' is not defined. This is confirmed by Ruff F821.
🐛 Proposed fix
Add the import after the existing monai.data / monai.transforms imports:
+from monai.data.meta_tensor import MetaTensor🧰 Tools
🪛 Ruff (0.15.1)
[error] 69-69: Undefined name MetaTensor
(F821)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@monai/apps/reconstruction/transforms/dictionary.py` around lines 66 - 71, The
code references MetaTensor in the dictionary extraction logic (used in the block
that checks self.image_only and calls isinstance(extracted_value, MetaTensor)
when populating d[key]) but MetaTensor is not imported; fix by adding the proper
import (e.g., add "from monai.data import MetaTensor" alongside the existing
monai imports) so that the isinstance check in that extraction loop (the code
around extracted_value = d[self.meta_key][key] and the self.image_only branch)
does not raise NameError at runtime.
| if SummaryWriter is None: | ||
| raise RuntimeError( | ||
| "TensorBoardHandler requires tensorboard to be installed. " | ||
| "Please install it with: pip install tensorboard" | ||
| ) |
There was a problem hiding this comment.
SummaryWriter is None is always False — the guard is dead code.
optional_import returns the imported module and a boolean flag indicating whether the import is successful. When tensorboard is missing, importing-related exceptions are stored and raised lazily when attempting to use the failed-to-import module — the returned object is a stub, never None. So SummaryWriter is None never triggers; the RuntimeError is never raised and the helpful message never shown. The stub's OptionalImportError fires instead at line 58.
Fix: retain the success flag from optional_import and check it.
🐛 Proposed fix
At line 34, capture the boolean:
- SummaryWriter, _ = optional_import("torch.utils.tensorboard", name="SummaryWriter")
+ SummaryWriter, _has_summary_writer = optional_import("torch.utils.tensorboard", name="SummaryWriter")Then in __init__:
- if SummaryWriter is None:
+ if not _has_summary_writer:
raise RuntimeError(
"TensorBoardHandler requires tensorboard to be installed. "
"Please install it with: pip install tensorboard"
)🧰 Tools
🪛 Ruff (0.15.1)
[warning] 54-57: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@monai/handlers/tensorboard_handlers.py` around lines 53 - 57, Capture the
boolean success flag returned by optional_import when importing SummaryWriter
(e.g., SummaryWriter, summary_writer_available = optional_import(...)) and store
that alongside the SummaryWriter symbol; then in TensorBoardHandler.__init__
check the boolean flag (summary_writer_available) instead of testing
"SummaryWriter is None" and raise the RuntimeError with the helpful install
message if the flag is False so the guard triggers before the
OptionalImportError stub is invoked.
- Adds image_only parameter to handle MetaTensor objects - Prevents redundant metadata extraction when image_only=True - Improves metadata handling for medical imaging workflows - Preserves MetaTensor metadata associations - Addresses issue Project-MONAI#7562 Signed-off-by: skdas20 <skdas5405@gmail.com>
skdas20
force-pushed
the
enhance/metatensor-extraction
branch
from
b69f75b to
5f84f35
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monai/apps/reconstruction/transforms/dictionary.py`:
- Around line 67-71: The current loop in the transform silently skips writing
d[key] when self.image_only is True and extracted_value is a MetaTensor (using
isinstance(extracted_value, MetaTensor)), which can cause downstream KeyError;
change this behavior in the method that builds d (referencing self.image_only,
extracted_value, MetaTensor, d[key], and self.meta_key) to either (1) explicitly
log a warning (processLogger or logger.warning) indicating the key was skipped
and why, or (2) raise a clear exception (e.g., ValueError) when image_only=True
would drop a MetaTensor so callers notice, and update the docstring to explain
the contract; additionally, rename the parameter self.image_only to a clearer
name such as self.skip_meta_tensor (and update all usages and docstrings) to
avoid the semantic confusion that MetaTensor is the image object.
- Around line 40-46: The docstring for ExtractDataKeyFromMetaKeyd incorrectly
describes image_only; update the parameter description and example to reflect
the actual behavior: when image_only=True the transform skips/excludes
MetaTensor values (does not extract their metadata) and only extracts metadata
from non-MetaTensor entries, and when image_only=False it will extract from
MetaTensor items as well; edit the parameter line mentioning image_only and the
example text to explicitly state this skip behavior and show a concrete fastMRI
example where "reconstruction_rss" remains in meta when image_only=True and is
moved to data when image_only=False.
- Around line 49-52: Import MetaTensor from monai.data.meta_tensor and update
the transform to reference it properly so using image_only=True doesn't raise
NameError: add "from monai.data.meta_tensor import MetaTensor" near other
imports and ensure the __init__ signature (def __init__(..., image_only: bool =
False)) and logic that checks MetaTensor use the imported name. Add unit tests
for the transform covering both image_only=False and image_only=True: create
test cases using a MetaTensor value and a plain torch tensor to verify the
extraction path when image_only=False and the skip/retain behavior when
image_only=True; assert the meta_key is extracted for MetaTensor and unchanged
for plain tensors. Ensure tests exercise the transform class/method that
performs meta extraction (the class containing __init__ and the method that
handles meta_key extraction) and include both positive and negative assertions
for MetaTensor vs plain tensor inputs.
---
Duplicate comments:
In `@monai/apps/reconstruction/transforms/dictionary.py`:
- Line 69: The NameError occurs because MetaTensor is referenced in the
conditional (the check "if self.image_only and isinstance(extracted_value,
MetaTensor):") but never imported; fix by adding the appropriate top-level
import for MetaTensor (e.g., add "from monai.data import MetaTensor") so the
isinstance check can resolve, and ensure any linter/order-of-import conventions
are respected.
| image_only: if True, only extract metadata from MetaTensor images to avoid duplication | ||
|
|
||
| Example: | ||
| When the fastMRI dataset is loaded, "kspace" is stored in the data dictionary, | ||
| but the ground-truth image with the key "reconstruction_rss" is stored in the meta data. | ||
| In this case, ExtractDataKeyFromMetaKeyd moves "reconstruction_rss" to data. | ||
| For MetaTensor objects, setting image_only=True prevents extracting redundant metadata. |
There was a problem hiding this comment.
Docstring for image_only inverts the actual behavior.
Line 40 says "only extract metadata from MetaTensor images", but the real effect is the opposite: MetaTensor values are skipped (not extracted). The example note on line 46 is also vague. Consider:
- image_only: if True, only extract metadata from MetaTensor images to avoid duplication
+ image_only: if True, skip keys whose value is a MetaTensor (leaves them in meta_key only).- For MetaTensor objects, setting image_only=True prevents extracting redundant metadata.
+ When image_only=True, MetaTensor-valued keys are not moved to the top-level data dict.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| image_only: if True, only extract metadata from MetaTensor images to avoid duplication | |
| Example: | |
| When the fastMRI dataset is loaded, "kspace" is stored in the data dictionary, | |
| but the ground-truth image with the key "reconstruction_rss" is stored in the meta data. | |
| In this case, ExtractDataKeyFromMetaKeyd moves "reconstruction_rss" to data. | |
| For MetaTensor objects, setting image_only=True prevents extracting redundant metadata. | |
| image_only: if True, skip keys whose value is a MetaTensor (leaves them in meta_key only). | |
| Example: | |
| When the fastMRI dataset is loaded, "kspace" is stored in the data dictionary, | |
| but the ground-truth image with the key "reconstruction_rss" is stored in the meta data. | |
| In this case, ExtractDataKeyFromMetaKeyd moves "reconstruction_rss" to data. | |
| When image_only=True, MetaTensor-valued keys are not moved to the top-level data dict. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@monai/apps/reconstruction/transforms/dictionary.py` around lines 40 - 46, The
docstring for ExtractDataKeyFromMetaKeyd incorrectly describes image_only;
update the parameter description and example to reflect the actual behavior:
when image_only=True the transform skips/excludes MetaTensor values (does not
extract their metadata) and only extracts metadata from non-MetaTensor entries,
and when image_only=False it will extract from MetaTensor items as well; edit
the parameter line mentioning image_only and the example text to explicitly
state this skip behavior and show a concrete fastMRI example where
"reconstruction_rss" remains in meta when image_only=True and is moved to data
when image_only=False.
| def __init__(self, keys: KeysCollection, meta_key: str, allow_missing_keys: bool = False, image_only: bool = False) -> None: | ||
| MapTransform.__init__(self, keys, allow_missing_keys) | ||
| self.meta_key = meta_key | ||
| self.image_only = image_only |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for test files and test coverage related to ExtractDataKeyFromMetaKeyd
find . -type f -name "*test*dictionary*" | head -20Repository: Project-MONAI/MONAI
Length of output: 45
🏁 Script executed:
# Search for references to ExtractDataKeyFromMetaKeyd in the codebase
rg "ExtractDataKeyFromMetaKeyd" --type py -lRepository: Project-MONAI/MONAI
Length of output: 113
🏁 Script executed:
# Search for image_only parameter usage/tests
rg "image_only" --type py -B 2 -A 2Repository: Project-MONAI/MONAI
Length of output: 50375
🏁 Script executed:
# Examine the implementation file to understand the parameter
head -100 monai/apps/reconstruction/transforms/dictionary.pyRepository: Project-MONAI/MONAI
Length of output: 4812
🏁 Script executed:
# Look for test files in reconstruction directory
find . -path "*reconstruction*test*" -type f -name "*.py"Repository: Project-MONAI/MONAI
Length of output: 485
🏁 Script executed:
# Search for any references to ExtractDataKeyFromMetaKeyd in the entire codebase
rg "ExtractDataKeyFromMetaKeyd" -B 3 -A 3Repository: Project-MONAI/MONAI
Length of output: 2000
🏁 Script executed:
# Examine the specific transform file more carefully to see the full implementation
sed -n '40,100p' monai/apps/reconstruction/transforms/dictionary.pyRepository: Project-MONAI/MONAI
Length of output: 3131
🏁 Script executed:
# Search for any tests that reference ExtractDataKeyFromMetaKeyd
rg "ExtractDataKeyFromMetaKeyd" tests/Repository: Project-MONAI/MONAI
Length of output: 45
🏁 Script executed:
# Check if MetaTensor is imported in the dictionary.py file
head -30 monai/apps/reconstruction/transforms/dictionary.pyRepository: Project-MONAI/MONAI
Length of output: 1342
🏁 Script executed:
# Check the full imports and look for MetaTensor import
cat monai/apps/reconstruction/transforms/dictionary.py | head -50Repository: Project-MONAI/MONAI
Length of output: 2417
🏁 Script executed:
# Search for MetaTensor import in the file
rg "MetaTensor" monai/apps/reconstruction/transforms/dictionary.pyRepository: Project-MONAI/MONAI
Length of output: 420
🏁 Script executed:
# Check if MetaTensor needs to be imported - search where it's defined
rg "class MetaTensor" monai/Repository: Project-MONAI/MONAI
Length of output: 129
🏁 Script executed:
# Look at the full __call__ method to understand the context better
sed -n '50,80p' monai/apps/reconstruction/transforms/dictionary.pyRepository: Project-MONAI/MONAI
Length of output: 1288
🏁 Script executed:
# Check the entire dictionary.py file for MetaTensor imports
cat monai/apps/reconstruction/transforms/dictionary.py | grep -n "MetaTensor\|import"Repository: Project-MONAI/MONAI
Length of output: 1175
🏁 Script executed:
# Check if MetaTensor is used anywhere in monai.data that we can import from
rg "from monai.data" monai/apps/reconstruction/transforms/dictionary.pyRepository: Project-MONAI/MONAI
Length of output: 45
🏁 Script executed:
# Look at similar dictionary transforms to see how they handle MetaTensor
rg "MetaTensor" monai/transforms/io/dictionary.py -B 2 -A 2 | head -20Repository: Project-MONAI/MONAI
Length of output: 45
🏁 Script executed:
# Check what MetaTensor import is needed
rg "from.*MetaTensor" monai/transforms/ | head -5Repository: Project-MONAI/MONAI
Length of output: 605
🏁 Script executed:
# Verify that MetaTensor is referenced but not imported by checking if code would fail
python3 << 'EOF'
import ast
import sys
with open('monai/apps/reconstruction/transforms/dictionary.py', 'r') as f:
content = f.read()
# Simple check: look for MetaTensor in code but not in imports
has_metatensor_import = 'from monai.data.meta_tensor import MetaTensor' in content
has_metatensor_usage = 'isinstance(extracted_value, MetaTensor)' in content
print(f"MetaTensor imported: {has_metatensor_import}")
print(f"MetaTensor used in code: {has_metatensor_usage}")
print(f"Will cause NameError if image_only=True is used: {has_metatensor_usage and not has_metatensor_import}")
EOFRepository: Project-MONAI/MONAI
Length of output: 173
🏁 Script executed:
# Check if there are ANY tests for ExtractDataKeyFromMetaKeyd
find tests/ -type f -name "*.py" -exec grep -l "ExtractDataKeyFromMetaKeyd" {} \;Repository: Project-MONAI/MONAI
Length of output: 45
Add missing MetaTensor import and implement unit tests for the image_only parameter.
MetaTensor is referenced in line 69 but not imported—the code will raise NameError when image_only=True. Import from monai.data.meta_tensor. Additionally, add test coverage for both image_only=False (default) and image_only=True with MetaTensor and plain tensor values to verify the skip and extract paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@monai/apps/reconstruction/transforms/dictionary.py` around lines 49 - 52,
Import MetaTensor from monai.data.meta_tensor and update the transform to
reference it properly so using image_only=True doesn't raise NameError: add
"from monai.data.meta_tensor import MetaTensor" near other imports and ensure
the __init__ signature (def __init__(..., image_only: bool = False)) and logic
that checks MetaTensor use the imported name. Add unit tests for the transform
covering both image_only=False and image_only=True: create test cases using a
MetaTensor value and a plain torch tensor to verify the extraction path when
image_only=False and the skip/retain behavior when image_only=True; assert the
meta_key is extracted for MetaTensor and unchanged for plain tensors. Ensure
tests exercise the transform class/method that performs meta extraction (the
class containing __init__ and the method that handles meta_key extraction) and
include both positive and negative assertions for MetaTensor vs plain tensor
inputs.
| # When image_only is True, skip if the extracted value is a MetaTensor | ||
| # to preserve metadata associations | ||
| if self.image_only and isinstance(extracted_value, MetaTensor): | ||
| continue | ||
| d[key] = extracted_value # type: ignore |
There was a problem hiding this comment.
Silent continue silently drops expected keys when image_only=True.
If image_only=True and a requested key's value is a MetaTensor, the key is never written into d. Any downstream transform accessing d[key] will get a KeyError with no warning. Either document this contract explicitly (the key stays in d[self.meta_key] only) or raise/log when a key is silently skipped.
Additionally, the parameter name image_only is semantically misleading — MetaTensor is the image object, so image_only=True skipping MetaTensors reads backwards. A name like skip_meta_tensor or non_tensor_only would be less ambiguous.
🧰 Tools
🪛 Ruff (0.15.1)
[error] 69-69: Undefined name MetaTensor
(F821)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@monai/apps/reconstruction/transforms/dictionary.py` around lines 67 - 71, The
current loop in the transform silently skips writing d[key] when self.image_only
is True and extracted_value is a MetaTensor (using isinstance(extracted_value,
MetaTensor)), which can cause downstream KeyError; change this behavior in the
method that builds d (referencing self.image_only, extracted_value, MetaTensor,
d[key], and self.meta_key) to either (1) explicitly log a warning (processLogger
or logger.warning) indicating the key was skipped and why, or (2) raise a clear
exception (e.g., ValueError) when image_only=True would drop a MetaTensor so
callers notice, and update the docstring to explain the contract; additionally,
rename the parameter self.image_only to a clearer name such as
self.skip_meta_tensor (and update all usages and docstrings) to avoid the
semantic confusion that MetaTensor is the image object.
1 participant
Enhanced [ExtractDataKeyFromMetaKeyd] with [image_only] parameter
Properly handles MetaTensor metadata without duplication