[MSD-350][feat] Ensure pyramidal tiff on import#3407
Open
tmoerkerken wants to merge 1 commit intodelmic:masterfrom
Open
[MSD-350][feat] Ensure pyramidal tiff on import#3407tmoerkerken wants to merge 1 commit intodelmic:masterfrom
tmoerkerken wants to merge 1 commit intodelmic:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds pyramidal TIFF support: detection and conversion functions in src/odemis/dataio/tiff.py ( Sequence Diagram(s)sequenceDiagram
actor User
participant AnalysisTab as Analysis/Correlation Tab
participant GUI_Helper as ensure_pyramidal_tiff_for_file/tileset_gui
participant Core_Util as ensure_pyramidal_tiff
participant Detector as is_pyramidal (tiff.py)
participant Converter as convert_to_pyramidal (tiff.py)
participant FS as File System
User->>AnalysisTab: Load TIFF(s)
AnalysisTab->>GUI_Helper: filename(s), parent, main_data
GUI_Helper->>Core_Util: filename, project_folder, standalone_mode
Core_Util->>Detector: is_pyramidal(filename)
Detector->>FS: read TIFF metadata (TIFFTAG_SUBIFD)
FS-->>Detector: metadata
alt already pyramidal
Detector-->>Core_Util: true
Core_Util-->>GUI_Helper: return original path
else needs conversion
Detector-->>Core_Util: false
Core_Util->>Converter: convert_to_pyramidal(src, dst, compression)
Converter->>FS: read image data
FS-->>Converter: image bytes
Converter->>FS: write pyramidal TIFF
FS-->>Converter: write complete
Converter-->>Core_Util: conversion done
Core_Util-->>GUI_Helper: return converted path
end
GUI_Helper-->>AnalysisTab: pyramidal path(s)
AnalysisTab->>FS: open pyramidal TIFF(s)
FS-->>AnalysisTab: image data
AnalysisTab-->>User: display/continue processing
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ 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.
🧹 Nitpick comments (11)
src/odemis/dataio/tiff.py (2)
2404-2420: Preserve exception chain withraise ... from exc.When re-raising exceptions, use
raise ... from excto preserve the original traceback and provide better debugging context.Proposed fix
except Exception as exc: - raise IOError("Failed to inspect TIFF pyramid status for %s: %s" % (filename, exc)) + raise IOError("Failed to inspect TIFF pyramid status for %s: %s" % (filename, exc)) from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/dataio/tiff.py` around lines 2404 - 2420, The except block in is_pyramidal currently re-raises a new IOError without preserving the original exception chain; modify the exception handling in the is_pyramidal function so that the raised IOError includes the original exception using "raise ... from exc" (i.e., raise IOError("Failed to inspect TIFF pyramid status for %s: %s" % (filename, exc)) from exc) to preserve the original traceback and debugging context.
2423-2437: Preserve exception chain withraise ... from exc.When re-raising exceptions, use
raise ... from excto preserve the original traceback. Additionally, note that this conversion does not preserve any thumbnail that may exist in the source file.Proposed fix
except Exception as exc: raise IOError("Failed to convert TIFF %s to pyramidal %s: %s" % - (src_filename, dst_filename, exc)) + (src_filename, dst_filename, exc)) from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/dataio/tiff.py` around lines 2423 - 2437, In convert_to_pyramidal, update the exception re-raise to preserve the original traceback by using "raise IOError(... ) from exc" instead of the current plain raise; also add a short note to the function docstring (or comment) that conversion does not preserve any source thumbnail so callers are aware the thumbnail is lost.src/odemis/gui/cont/tabs/analysis_tab.py (2)
331-335: Consider adding type hints and docstring.This method lacks type hints for parameters and return type, as well as a docstring. As per coding guidelines, these should be included for all functions.
Proposed signature with type hints
- def load_data(self, filename, fmt=None, extend=False): + def load_data(self, filename: str, fmt: str | None = None, extend: bool = False) -> None: + """ + Load data from a file. + + :param filename: Path to the file to load + :param fmt: Format of the file, or None to auto-detect + :param extend: If True, extend current streams; if False, replace them + """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/tabs/analysis_tab.py` around lines 331 - 335, The load_data method is missing type hints and a docstring; update the signature of load_data(filename, fmt=None, extend=False) to include type hints (e.g., filename: str, fmt: Optional[str] = None, extend: bool = False) and a return type of -> None, add a concise docstring describing parameters and effects (that it ensures pyramidal TIFF via ensure_pyramidal_tiff_for_file_gui, opens acquisition via open_acquisition, and calls display_new_data), and add "from typing import Optional" at the top if not already imported so the type annotations are valid; keep references to ensure_pyramidal_tiff_for_file_gui, open_acquisition, and display_new_data in the docstring for clarity.
325-329: Consider adding type hints and docstring.This method lacks type hints for parameters and return type, as well as a docstring. As per coding guidelines, these should be included for all functions.
Proposed signature with type hints
- def load_tileset(self, filenames, extend=False): + def load_tileset(self, filenames: list[str], extend: bool = False) -> None: + """ + Load and stitch a tileset of files. + + :param filenames: List of file paths to load as tileset + :param extend: If True, extend current streams; if False, replace them + """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/tabs/analysis_tab.py` around lines 325 - 329, Add a clear docstring and type hints to load_tileset: change the signature of load_tileset to include types (e.g., filenames: Sequence[str] or List[str], extend: bool = False) and an explicit return type of -> None; update the docstring to briefly describe what the method does, list parameters (filenames: list of file paths, extend: whether to append vs replace), and note side effects (calls ensure_pyramidal_tiff_for_tileset_gui, open_files_and_stitch, and display_new_data) and any exceptions that may be raised. Ensure the docstring mentions that TIFFs are converted to pyramidal and that the first filename is used in display_new_data(filenames[0], ...). Keep wording concise and follow existing project docstring style.src/odemis/gui/cont/tabs/correlation_tab.py (2)
179-183: Fix implicitOptionaltype hint.The
fmtparameter uses implicitOptionalwhich is prohibited by PEP 484. Use explicit union syntax instead.Proposed fix
- def load_data(self, filename: str, fmt: str = None, extend: bool = False) -> None: + def load_data(self, filename: str, fmt: str | None = None, extend: bool = False) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/tabs/correlation_tab.py` around lines 179 - 183, The type hint for the parameter fmt in the method load_data uses an implicit Optional (e.g. "fmt: str = None"), which violates PEP 484; update the signature of load_data to use an explicit union such as fmt: Optional[str] or fmt: Union[str, None] (and import typing.Optional or typing.Union at the top if not already present) so the function definition def load_data(self, filename: str, fmt: Optional[str] = None, extend: bool = False) -> None is explicit; the rest of the method (ensure_pyramidal_tiff_for_file_gui, open_acquisition, load_streams) remains unchanged.
173-177: Consider adding a docstring.This method lacks a docstring describing its purpose, parameters, and behavior. As per coding guidelines, docstrings should be included for all functions following reStructuredText style.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/tabs/correlation_tab.py` around lines 173 - 177, Add a reStructuredText docstring to the load_tileset method that succinctly describes its purpose (load and stitch a tileset into the GUI), the parameters (filenames: List[str] — list of file paths; extend: bool — whether to extend existing tileset behavior), and behavior/side-effects (converts TIFFs to pyramidal via ensure_pyramidal_tiff_for_tileset_gui, stitches/opens files with open_files_and_stitch, and passes resulting data to load_streams); mention any notable TODO (user-defined registration/weave not yet supported) and the fact that extend is currently unused or how it should affect loading. Include parameter and return sections in reST style and a brief note about raised exceptions if applicable.src/odemis/util/tiff_convert.py (3)
47-51: Use exception chaining to preserve traceback.When re-raising an exception, use
raise ... from eto preserve the original traceback for debugging.♻️ Add exception chaining
try: return tiff.is_pyramidal(filename) except Exception as e: logger.error(f"Error checking if TIFF is pyramidal: {e}") - raise IOError(f"Failed to check TIFF pyramid status for {filename}: {e}") + raise IOError(f"Failed to check TIFF pyramid status for {filename}: {e}") from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/tiff_convert.py` around lines 47 - 51, The except block in the function calling tiff.is_pyramidal currently logs and re-raises an IOError without chaining the original exception; modify the re-raise to use exception chaining (raise IOError(f"Failed to check TIFF pyramid status for {filename}: {e}") from e) so the original traceback is preserved, keeping the existing logger.error call and using the same variables (tiff.is_pyramidal, logger, filename, e).
192-197: Use exception chaining to preserve traceback.When re-raising unexpected exceptions, use
raise ... from eto preserve the original traceback.♻️ Add exception chaining
except IOError: # Re-raise IOError as-is raise except Exception as e: logger.error(f"Unexpected error in ensure_pyramidal_tiff: {e}") - raise IOError(f"Unexpected error processing TIFF file: {e}") + raise IOError(f"Unexpected error processing TIFF file: {e}") from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/tiff_convert.py` around lines 192 - 197, The except block in ensure_pyramidal_tiff catches Exception as e and raises a new IOError but loses the original traceback; change the re-raise to use exception chaining (raise IOError(f"Unexpected error processing TIFF file: {e}") from e) so the original exception and traceback are preserved, and keep the logger.error call as-is to log the message before re-raising.
135-143: Use exception chaining to preserve traceback.When re-raising an exception, use
raise ... from eto preserve the original traceback.♻️ Add exception chaining
except Exception as e: logger.error(f"Error converting TIFF to pyramidal format: {e}") # Clean up partial file if it exists if os.path.exists(dst_filename): try: os.remove(dst_filename) except Exception as cleanup_error: logger.warning(f"Failed to clean up partial file {dst_filename}: {cleanup_error}") - raise IOError(f"Failed to convert TIFF file: {e}") + raise IOError(f"Failed to convert TIFF file: {e}") from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/tiff_convert.py` around lines 135 - 143, In the except block inside the TIFF conversion function (the except Exception as e handling in tiff_convert.py), change the re-raise to use exception chaining so the original traceback is preserved: after logging the error and performing the cleanup of dst_filename, raise the new IOError using "from e" (i.e., raise IOError(f"Failed to convert TIFF file: {e}") from e) instead of a plain raise; keep the existing logger.error and cleanup logic intact and only modify the final raise to chain the original exception.src/odemis/dataio/test/tiff_test.py (1)
55-66: Consider usingshutil.rmtreefor simpler cleanup.The manual file-by-file cleanup works but could be simplified. Also, the bare
except Exception: passsilently swallows errors which may hide issues during test debugging.♻️ Optional: Simplified cleanup with shutil
+import shutil + class TestTiffPyramidalHelpers(unittest.TestCase): def setUp(self) -> None: self.temp_dir = tempfile.mkdtemp() def tearDown(self) -> None: - try: - for filename in os.listdir(self.temp_dir): - file_path = os.path.join(self.temp_dir, filename) - if os.path.isfile(file_path): - os.remove(file_path) - os.rmdir(self.temp_dir) - except Exception: - pass + shutil.rmtree(self.temp_dir, ignore_errors=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/dataio/test/tiff_test.py` around lines 55 - 66, Replace the manual file-by-file removal in tearDown with shutil.rmtree(self.temp_dir) to simplify cleanup (use the tempfile-created directory stored in self.temp_dir), and remove the bare "except Exception: pass" so errors aren't silently swallowed—either let exceptions propagate or catch and re-raise/log them (refer to the setUp and tearDown methods and the self.temp_dir attribute).src/odemis/util/test/tiff_convert_test.py (1)
38-52: Consider usingshutil.rmtreefor simpler cleanup.Same pattern as in
tiff_test.py. The bareexcept Exception: passsilently swallows cleanup errors.♻️ Optional: Simplified cleanup with shutil
+import shutil + def tearDown(self) -> None: """Clean up temporary files.""" - # Remove all temporary files - try: - for filename in os.listdir(self.temp_dir): - file_path = os.path.join(self.temp_dir, filename) - if os.path.isfile(file_path): - os.remove(file_path) - os.rmdir(self.temp_dir) - except Exception: - pass + shutil.rmtree(self.temp_dir, ignore_errors=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/test/tiff_convert_test.py` around lines 38 - 52, Replace the manual file-by-file cleanup in tearDown with shutil.rmtree to remove the temp directory in one call: import shutil, then in tearDown call shutil.rmtree(self.temp_dir) instead of iterating and os.remove; remove the bare "except Exception: pass" (or if you must catch errors, catch OSError/PermissionError and handle or re-raise) so failures aren't silently swallowed; the change affects the setUp/tearDown methods and the self.temp_dir usage in this test file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/odemis/dataio/test/tiff_test.py`:
- Around line 55-66: Replace the manual file-by-file removal in tearDown with
shutil.rmtree(self.temp_dir) to simplify cleanup (use the tempfile-created
directory stored in self.temp_dir), and remove the bare "except Exception: pass"
so errors aren't silently swallowed—either let exceptions propagate or catch and
re-raise/log them (refer to the setUp and tearDown methods and the self.temp_dir
attribute).
In `@src/odemis/dataio/tiff.py`:
- Around line 2404-2420: The except block in is_pyramidal currently re-raises a
new IOError without preserving the original exception chain; modify the
exception handling in the is_pyramidal function so that the raised IOError
includes the original exception using "raise ... from exc" (i.e., raise
IOError("Failed to inspect TIFF pyramid status for %s: %s" % (filename, exc))
from exc) to preserve the original traceback and debugging context.
- Around line 2423-2437: In convert_to_pyramidal, update the exception re-raise
to preserve the original traceback by using "raise IOError(... ) from exc"
instead of the current plain raise; also add a short note to the function
docstring (or comment) that conversion does not preserve any source thumbnail so
callers are aware the thumbnail is lost.
In `@src/odemis/gui/cont/tabs/analysis_tab.py`:
- Around line 331-335: The load_data method is missing type hints and a
docstring; update the signature of load_data(filename, fmt=None, extend=False)
to include type hints (e.g., filename: str, fmt: Optional[str] = None, extend:
bool = False) and a return type of -> None, add a concise docstring describing
parameters and effects (that it ensures pyramidal TIFF via
ensure_pyramidal_tiff_for_file_gui, opens acquisition via open_acquisition, and
calls display_new_data), and add "from typing import Optional" at the top if not
already imported so the type annotations are valid; keep references to
ensure_pyramidal_tiff_for_file_gui, open_acquisition, and display_new_data in
the docstring for clarity.
- Around line 325-329: Add a clear docstring and type hints to load_tileset:
change the signature of load_tileset to include types (e.g., filenames:
Sequence[str] or List[str], extend: bool = False) and an explicit return type of
-> None; update the docstring to briefly describe what the method does, list
parameters (filenames: list of file paths, extend: whether to append vs
replace), and note side effects (calls ensure_pyramidal_tiff_for_tileset_gui,
open_files_and_stitch, and display_new_data) and any exceptions that may be
raised. Ensure the docstring mentions that TIFFs are converted to pyramidal and
that the first filename is used in display_new_data(filenames[0], ...). Keep
wording concise and follow existing project docstring style.
In `@src/odemis/gui/cont/tabs/correlation_tab.py`:
- Around line 179-183: The type hint for the parameter fmt in the method
load_data uses an implicit Optional (e.g. "fmt: str = None"), which violates PEP
484; update the signature of load_data to use an explicit union such as fmt:
Optional[str] or fmt: Union[str, None] (and import typing.Optional or
typing.Union at the top if not already present) so the function definition def
load_data(self, filename: str, fmt: Optional[str] = None, extend: bool = False)
-> None is explicit; the rest of the method (ensure_pyramidal_tiff_for_file_gui,
open_acquisition, load_streams) remains unchanged.
- Around line 173-177: Add a reStructuredText docstring to the load_tileset
method that succinctly describes its purpose (load and stitch a tileset into the
GUI), the parameters (filenames: List[str] — list of file paths; extend: bool —
whether to extend existing tileset behavior), and behavior/side-effects
(converts TIFFs to pyramidal via ensure_pyramidal_tiff_for_tileset_gui,
stitches/opens files with open_files_and_stitch, and passes resulting data to
load_streams); mention any notable TODO (user-defined registration/weave not yet
supported) and the fact that extend is currently unused or how it should affect
loading. Include parameter and return sections in reST style and a brief note
about raised exceptions if applicable.
In `@src/odemis/util/test/tiff_convert_test.py`:
- Around line 38-52: Replace the manual file-by-file cleanup in tearDown with
shutil.rmtree to remove the temp directory in one call: import shutil, then in
tearDown call shutil.rmtree(self.temp_dir) instead of iterating and os.remove;
remove the bare "except Exception: pass" (or if you must catch errors, catch
OSError/PermissionError and handle or re-raise) so failures aren't silently
swallowed; the change affects the setUp/tearDown methods and the self.temp_dir
usage in this test file.
In `@src/odemis/util/tiff_convert.py`:
- Around line 47-51: The except block in the function calling tiff.is_pyramidal
currently logs and re-raises an IOError without chaining the original exception;
modify the re-raise to use exception chaining (raise IOError(f"Failed to check
TIFF pyramid status for {filename}: {e}") from e) so the original traceback is
preserved, keeping the existing logger.error call and using the same variables
(tiff.is_pyramidal, logger, filename, e).
- Around line 192-197: The except block in ensure_pyramidal_tiff catches
Exception as e and raises a new IOError but loses the original traceback; change
the re-raise to use exception chaining (raise IOError(f"Unexpected error
processing TIFF file: {e}") from e) so the original exception and traceback are
preserved, and keep the logger.error call as-is to log the message before
re-raising.
- Around line 135-143: In the except block inside the TIFF conversion function
(the except Exception as e handling in tiff_convert.py), change the re-raise to
use exception chaining so the original traceback is preserved: after logging the
error and performing the cleanup of dst_filename, raise the new IOError using
"from e" (i.e., raise IOError(f"Failed to convert TIFF file: {e}") from e)
instead of a plain raise; keep the existing logger.error and cleanup logic
intact and only modify the final raise to chain the original exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 807fb99f-1169-4b88-bb65-afe0c922a68c
📒 Files selected for processing (6)
src/odemis/dataio/test/tiff_test.pysrc/odemis/dataio/tiff.pysrc/odemis/gui/cont/tabs/analysis_tab.pysrc/odemis/gui/cont/tabs/correlation_tab.pysrc/odemis/util/test/tiff_convert_test.pysrc/odemis/util/tiff_convert.py
tmoerkerken
force-pushed
the
force-pyramidal-tiff-converison
branch
from
20b12a2 to
7bef02b
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
src/odemis/util/test/tiff_convert_test.py (1)
36-36: Add a class-level docstring forTestTiffConversion.The new test class is missing a class docstring.
As per coding guidelines, "Include docstrings for all functions and classes, following the reStructuredText style guide (without type information)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/test/tiff_convert_test.py` at line 36, Add a class-level reStructuredText-style docstring to the TestTiffConversion unittest.TestCase class describing its purpose (e.g., "Tests for TIFF conversion utilities"), placed directly under the class declaration in TestTiffConversion; keep it brief, descriptive, and without type information or parameter details to satisfy the project's docstring guideline.src/odemis/dataio/test/tiff_test.py (1)
53-95: Add docstrings for the new test class and methods.The newly introduced
TestTiffPyramidalIOblock lacks class/method docstrings in this added section.As per coding guidelines, "Include docstrings for all functions and classes, following the reStructuredText style guide (without type information)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/dataio/test/tiff_test.py` around lines 53 - 95, The test class TestTiffPyramidalIO and its methods (setUp, tearDown, _create_test_tiff, test_is_pyramidal_tiff, test_is_pyramidal_non_pyramidal_tiff, test_is_pyramidal_invalid_file, test_convert_to_pyramidal_tiff) are missing docstrings; add concise reStructuredText-style docstrings (no type info) to the class and each public method explaining their purpose and behavior (for example: class-level summary of what the test suite covers and one-line descriptions for setUp/tearDown helpers and each test method), placed immediately below the def/class lines. Ensure docstrings describe intent and expected outcomes but do not include type annotations.src/odemis/gui/cont/tabs/correlation_tab.py (1)
173-181: Add docstrings to touched loader methods.
load_tilesetandload_dataare touched but still undocumented (Line 173 and Line 179), which misses the repository docstring requirement.As per coding guidelines, "Include docstrings for all functions and classes, following the reStructuredText style guide (without type information)".💡 Suggested docstring update
def load_tileset(self, filenames: List[str], extend: bool = False) -> None: + """ + Load and stitch a tileset into correlation streams. + + :param filenames: Selected file paths + :param extend: Whether to append to existing streams + """ # Convert all files to pyramidal if they're TIFFs filenames = ensure_pyramidal_tiff_for_tileset_gui(filenames, self.panel, self.main_data) data = open_files_and_stitch(filenames) # TODO: allow user defined registration / weave methods self.load_streams(data) -def load_data(self, filename: str, fmt: str = None, extend: bool = False) -> None: +def load_data(self, filename: str, fmt: str = None, extend: bool = False) -> None: + """ + Load one acquisition file into correlation streams. + + :param filename: Selected file path + :param fmt: Explicit format selected in file dialog + :param extend: Whether to append to existing streams + """ # Convert to pyramidal if it's a TIFF file filename = ensure_pyramidal_tiff_for_file_gui(filename, self.panel, self.main_data) data = open_acquisition(filename, fmt) self.load_streams(data)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/tabs/correlation_tab.py` around lines 173 - 181, Both loader methods lack docstrings: add reStructuredText-style docstrings (no type annotations) to load_tileset and load_data describing what each does, their parameters and behavior, and return value/side-effects; for load_tileset mention it converts TIFFs to pyramidal, stitches files and calls load_streams and document parameters filenames (list) and extend (bool) and side-effects on panel/main_data, and for load_data describe conversion to pyramidal, parameters filename, fmt and extend and that it mutates/returns None. Ensure short summary line, parameter sections like :param filename: and :param extend: and a :returns: or :raises: if applicable.src/odemis/util/tiff_convert.py (1)
63-63: Minor docstring inaccuracy: prefix is not "hidden".The docstring says "stores next to source file with hidden prefix" but the actual prefix is
converted_(line 81), not a dot-prefix like.converted_. On Unix systems, only dot-prefixed files are hidden.Proposed fix
- In standalone/Viewer mode, stores next to source file with hidden prefix. + In standalone/Viewer mode, stores next to source file with 'converted_' prefix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/tiff_convert.py` at line 63, The docstring that currently says "stores next to source file with hidden prefix" is inaccurate; update the docstring (the string describing behavior in src/odemis/util/tiff_convert.py) to state the actual prefix used ("converted_") instead of calling it "hidden" — e.g., change the phrase to "stores next to the source file with the prefix 'converted_'" or equivalent wording that does not imply a dot-hidden file; reference the actual prefix literal converted_ used in the function/logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/dataio/test/tiff_test.py`:
- Around line 58-66: The tearDown method currently swallows all exceptions,
hiding cleanup failures; change it to perform robust cleanup for self.temp_dir
(e.g., use shutil.rmtree(self.temp_dir) or iterate files) and only suppress
expected exceptions (FileNotFoundError when the dir/file is already gone) while
allowing or logging unexpected exceptions (OSError/PermissionError) to surface
or re-raise so test framework shows failures; update the tearDown implementation
in the tearDown method to catch specific exceptions and either log them or
re-raise instead of using a bare except.
In `@src/odemis/dataio/tiff.py`:
- Around line 2413-2437: The is_pyramidal function currently checks
TIFFTAG_SUBIFD only for the initial IFD and can miss pyramidal data in later
directories; update is_pyramidal to iterate through all IFDs/directories in the
opened TIFF (using the TIFF object's directory navigation methods, e.g.,
SetDirectory(index) or an iterator if available) and check TIFFTAG_SUBIFD
(and/or any other pyramid-indicating tag) on each directory, returning True if
any directory contains sub-IFDs; ensure the TIFF file is closed in a finally
block and propagate errors as before.
In `@src/odemis/util/test/tiff_convert_test.py`:
- Around line 42-52: The tearDown method currently swallows all exceptions with
a bare except/pass which hides problems; change it to perform deterministic
cleanup of self.temp_dir using shutil.rmtree (or check os.path.exists and remove
files) and catch only expected exceptions (e.g., FileNotFoundError,
PermissionError, OSError) and re-raise or let pytest fail on unexpected errors;
update the tearDown function reference to use shutil.rmtree(self.temp_dir,
ignore_errors=False) inside a try/except that only handles specific exceptions
and logs or fails the test instead of silently passing.
In `@src/odemis/util/tiff_convert.py`:
- Around line 135-143: The except block that logs and re-raises in tiff
conversion should preserve the original traceback by chaining the caught
exception; in the except Exception as e handler (the block referencing logger,
dst_filename cleanup and raising IOError), change the re-raise to use exception
chaining (raise IOError(f"Failed to convert TIFF file: {e}") from e) so the
original exception is attached and traceable while keeping the existing cleanup
and warning behavior.
- Around line 47-51: The except block in the tiff pyramid check currently logs
and re-raises an IOError losing the original traceback; update the handler in
the function that calls tiff.is_pyramidal to use exception chaining when
re-raising (raise IOError(...) from e) so the original exception is preserved,
while keeping the logger.error call that references filename and the caught
exception.
- Around line 195-197: The except block in ensure_pyramidal_tiff currently logs
and re-raises a new IOError without chaining the original exception; modify the
re-raise to preserve the original traceback by raising the new IOError from the
caught exception (i.e., use "raise IOError(... ) from e") so the original
exception context is retained while keeping the logger.error call.
- Around line 94-128: The compression parameter in convert_to_pyramidal is
treated only as a boolean but the docstring lists specific algorithms; either
propagate the chosen algorithm to the underlying call or clarify the docstring:
update the convert_to_pyramidal signature/usage to accept a compression string
(e.g., pass compression or compression.lower() to tiff.convert_to_pyramidal) and
adjust any helper logic that computes `compressed`, or change the function
docstring to state that only compression enable/disable is supported and keep
passing `compressed=compressed` to tiff.convert_to_pyramidal; reference the
function convert_to_pyramidal and the `compression` parameter in your change.
---
Nitpick comments:
In `@src/odemis/dataio/test/tiff_test.py`:
- Around line 53-95: The test class TestTiffPyramidalIO and its methods (setUp,
tearDown, _create_test_tiff, test_is_pyramidal_tiff,
test_is_pyramidal_non_pyramidal_tiff, test_is_pyramidal_invalid_file,
test_convert_to_pyramidal_tiff) are missing docstrings; add concise
reStructuredText-style docstrings (no type info) to the class and each public
method explaining their purpose and behavior (for example: class-level summary
of what the test suite covers and one-line descriptions for setUp/tearDown
helpers and each test method), placed immediately below the def/class lines.
Ensure docstrings describe intent and expected outcomes but do not include type
annotations.
In `@src/odemis/gui/cont/tabs/correlation_tab.py`:
- Around line 173-181: Both loader methods lack docstrings: add
reStructuredText-style docstrings (no type annotations) to load_tileset and
load_data describing what each does, their parameters and behavior, and return
value/side-effects; for load_tileset mention it converts TIFFs to pyramidal,
stitches files and calls load_streams and document parameters filenames (list)
and extend (bool) and side-effects on panel/main_data, and for load_data
describe conversion to pyramidal, parameters filename, fmt and extend and that
it mutates/returns None. Ensure short summary line, parameter sections like
:param filename: and :param extend: and a :returns: or :raises: if applicable.
In `@src/odemis/util/test/tiff_convert_test.py`:
- Line 36: Add a class-level reStructuredText-style docstring to the
TestTiffConversion unittest.TestCase class describing its purpose (e.g., "Tests
for TIFF conversion utilities"), placed directly under the class declaration in
TestTiffConversion; keep it brief, descriptive, and without type information or
parameter details to satisfy the project's docstring guideline.
In `@src/odemis/util/tiff_convert.py`:
- Line 63: The docstring that currently says "stores next to source file with
hidden prefix" is inaccurate; update the docstring (the string describing
behavior in src/odemis/util/tiff_convert.py) to state the actual prefix used
("converted_") instead of calling it "hidden" — e.g., change the phrase to
"stores next to the source file with the prefix 'converted_'" or equivalent
wording that does not imply a dot-hidden file; reference the actual prefix
literal converted_ used in the function/logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 16c2d6fb-0ee9-4ffd-aaf9-b9e162f168ab
📒 Files selected for processing (6)
src/odemis/dataio/test/tiff_test.pysrc/odemis/dataio/tiff.pysrc/odemis/gui/cont/tabs/analysis_tab.pysrc/odemis/gui/cont/tabs/correlation_tab.pysrc/odemis/util/test/tiff_convert_test.pysrc/odemis/util/tiff_convert.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/odemis/gui/cont/tabs/analysis_tab.py
tmoerkerken
force-pushed
the
force-pyramidal-tiff-converison
branch
from
7bef02b to
5da0453
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
src/odemis/dataio/tiff.py (2)
2419-2420:⚠️ Potential issue | 🟡 MinorPreserve traceback context when re-raising as
IOError.At Line 2420 and Line 2436, re-raising without
from excdrops the original exception chain and makes conversion failures harder to diagnose.Proposed fix
- except Exception as exc: - raise IOError("Failed to inspect TIFF pyramid status for %s: %s" % (filename, exc)) + except Exception as exc: + raise IOError("Failed to inspect TIFF pyramid status for %s: %s" % (filename, exc)) from exc @@ - except Exception as exc: - raise IOError("Failed to convert TIFF %s to pyramidal %s: %s" % - (src_filename, dst_filename, exc)) + except Exception as exc: + raise IOError("Failed to convert TIFF %s to pyramidal %s: %s" % + (src_filename, dst_filename, exc)) from excAlso applies to: 2435-2437
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/dataio/tiff.py` around lines 2419 - 2420, The except blocks that currently do "except Exception as exc: raise IOError(\"Failed to inspect TIFF pyramid status for %s: %s\" % (filename, exc))" (and the similar block at the later occurrence) are dropping the original traceback; update both to re-raise using exception chaining: "raise IOError(... ) from exc" so the original exception context is preserved when raising the IOError (apply to both occurrences that reference filename and exc).
2415-2416:⚠️ Potential issue | 🟠 MajorScan all IFDs before returning “not pyramidal.”
At Line 2415,
TIFFTAG_SUBIFDis checked only once on the current directory. Multi-IFD TIFFs can store sub-IFDs on later directories, which leads to false negatives and unnecessary reconversion.Proposed fix
def is_pyramidal(filename: str) -> bool: @@ try: tiff_file = TIFF.open(filename, mode="r") try: - sub_ifds = tiff_file.GetField(T.TIFFTAG_SUBIFD) - return sub_ifds is not None and len(sub_ifds) > 0 + tiff_file.SetDirectory(0) + while True: + sub_ifds = tiff_file.GetField(T.TIFFTAG_SUBIFD) + if sub_ifds: + return True + if tiff_file.LastDirectory(): + return False + tiff_file.ReadDirectory() finally: tiff_file.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/dataio/tiff.py` around lines 2415 - 2416, The code currently only checks TIFFTAG_SUBIFD on the current directory via tiff_file.GetField and returns “not pyramidal” too early; instead iterate over all IFDs/directories in the TIFF (using tiff_file.SetDirectory/NextDirectory or the library's directory iteration API) and check TIFFTAG_SUBIFD on each one, returning true if any directory has sub-IFDs and only returning false after all directories have been scanned; ensure you restore the original directory (or otherwise avoid leaving the tiff_file positioned on a different IFD) to prevent side effects.src/odemis/dataio/test/tiff_test.py (1)
58-66:⚠️ Potential issue | 🟡 MinorAvoid
except Exception: passin test cleanup.Line 65 suppresses all teardown errors and can mask residue between tests. Use deterministic cleanup (
shutil.rmtree) instead of silent pass.Proposed fix
+import shutil @@ def tearDown(self) -> None: - try: - for filename in os.listdir(self.temp_dir): - file_path = os.path.join(self.temp_dir, filename) - if os.path.isfile(file_path): - os.remove(file_path) - os.rmdir(self.temp_dir) - except Exception: - pass + shutil.rmtree(self.temp_dir)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/dataio/test/tiff_test.py` around lines 58 - 66, The teardown currently in tearDown swallows all exceptions (using except Exception: pass) and manually iterates os.listdir/os.remove/os.rmdir which can leave residue; replace this with a deterministic cleanup using shutil.rmtree on self.temp_dir (guarded by an existence check like os.path.exists(self.temp_dir)) and remove the broad exception suppression so failures are surfaced; update references in the test to stop using os.listdir/os.remove/os.rmdir and instead call shutil.rmtree(self.temp_dir) (or allow the exception to propagate) to ensure proper cleanup and test isolation.src/odemis/util/test/tiff_convert_test.py (1)
45-52:⚠️ Potential issue | 🟡 MinorDon’t swallow teardown failures with
except Exception: pass.This can hide filesystem cleanup problems and make test isolation flaky. Prefer deterministic cleanup with
shutil.rmtree.Proposed fix
+import shutil @@ def tearDown(self) -> None: """Clean up temporary files.""" - # Remove all temporary files - try: - for filename in os.listdir(self.temp_dir): - file_path = os.path.join(self.temp_dir, filename) - if os.path.isfile(file_path): - os.remove(file_path) - os.rmdir(self.temp_dir) - except Exception: - pass + shutil.rmtree(self.temp_dir)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/test/tiff_convert_test.py` around lines 45 - 52, Replace the silent teardown that walks self.temp_dir and swallows all exceptions with a deterministic remove using shutil.rmtree(self.temp_dir) (or shutil.rmtree(self.temp_dir, ignore_errors=True) if you intentionally want to ignore missing files), and do not use a bare "except Exception: pass"; instead let failures surface or explicitly handle expected OSErrors so test cleanup problems aren’t hidden. Update the teardown that currently iterates over os.listdir/self.temp_dir to call shutil.rmtree and reference self.temp_dir in the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/dataio/test/tiff_test.py`:
- Around line 53-95: The TestTiffPyramidalIO test class and its methods
(TestTiffPyramidalIO, setUp, tearDown, _create_test_tiff,
test_is_pyramidal_tiff, test_is_pyramidal_non_pyramidal_tiff,
test_is_pyramidal_invalid_file, test_convert_to_pyramidal_tiff) lack docstrings;
add short reStructuredText-style docstrings to the class and each public method
describing their purpose (e.g., class: what the test suite verifies;
setUp/tearDown: test env setup/cleanup; _create_test_tiff: helper to create a
test TIFF and parameters; each test_*: what scenario is being tested and
expected outcome) without including type information.
In `@src/odemis/dataio/tiff.py`:
- Around line 2433-2434: The current code calls read_data(src_filename) which
materializes all pages into memory before calling export(dst_filename, data,
compressed=compressed, pyramid=True); change this to stream pages instead of
loading them all: either update read_data to return an iterator/generator of
page arrays (not a list) or replace the read_data call with a TiffFile-based
page iterator (e.g., iterate tifffile.TiffFile(src_filename).pages and yield
numpy arrays) and pass that iterable to export so export consumes pages lazily;
keep the same arguments (dst_filename, compressed, pyramid) and ensure export
accepts an iterable of pages (or adapt export to accept an iterator) to prevent
full in-memory conversion for large TIFFs.
In `@src/odemis/gui/cont/tabs/analysis_tab.py`:
- Around line 325-335: The methods load_tileset and load_data lack type
annotations and docstrings; update their signatures to include type hints (e.g.,
def load_tileset(self, filenames: Sequence[str], extend: bool = False) -> None
and def load_data(self, filename: str, fmt: Optional[str] = None, extend: bool =
False) -> None) and import typing types (Sequence, Optional) as needed, then add
concise docstrings for each describing purpose, parameters (including types and
meanings for filenames/filename, fmt, extend), and that they return None; ensure
the docstrings mention side effects (calling
ensure_pyramidal_tiff_for_tileset_gui/ensure_pyramidal_tiff_for_file_gui,
open_files_and_stitch/open_acquisition, and display_new_data) so callers know
behavior.
In `@src/odemis/util/test/tiff_convert_test.py`:
- Line 36: Add a reStructuredText-style class docstring to TestTiffConversion
describing the purpose of the test class and its responsibilities (e.g., "Tests
for TIFF conversion utilities including X, Y, Z"), placed immediately under the
class definition in src/odemis/util/test/tiff_convert_test.py; follow the repo
guideline of not including type information in the docstring and keep it concise
but descriptive to satisfy the class docstring requirement for
TestTiffConversion.
In `@src/odemis/util/tiff_convert.py`:
- Around line 63-66: The current returned path for converted files in
normal/project mode uses only converted_{name_without_ext}{ext}, causing
collisions when different input TIFFs share a basename; update the logic in the
function that builds converted_filename (use the variables project_folder,
name_without_ext, ext) to produce a unique destination per source by
incorporating either a deterministic unique token (e.g., a short hash of the
original absolute path) or a relative folder component into the filename (e.g.,
converted_{name_without_ext}_{hash}.{ext} or
converted_{parentdir}_{name_without_ext}{ext}) before joining with
project_folder and returning the path; ensure the token is filesystem-safe and
keep the change local to the path construction so callers remain unchanged.
- Around line 275-304: Remove the manual tiff.is_pyramidal pre-check and its
try/except and instead always call ensure_pyramidal_tiff(filename,
project_folder=project_folder, standalone_mode=standalone,
progress_callback=progress_callback) for each file so inspection/conversion
flows through the same error-normalizing path; keep the existing
progress_callback, the IOError except block that logs via logger.error("Failed
to convert TIFF file %s: %s", filename, exc) and sets converted_file = filename,
and ensure the converted_files.append(converted_file) and dlg.Update(...)
bookkeeping still runs in the per-file loop so a failing inspection or
conversion never aborts the whole tileset import.
---
Duplicate comments:
In `@src/odemis/dataio/test/tiff_test.py`:
- Around line 58-66: The teardown currently in tearDown swallows all exceptions
(using except Exception: pass) and manually iterates
os.listdir/os.remove/os.rmdir which can leave residue; replace this with a
deterministic cleanup using shutil.rmtree on self.temp_dir (guarded by an
existence check like os.path.exists(self.temp_dir)) and remove the broad
exception suppression so failures are surfaced; update references in the test to
stop using os.listdir/os.remove/os.rmdir and instead call
shutil.rmtree(self.temp_dir) (or allow the exception to propagate) to ensure
proper cleanup and test isolation.
In `@src/odemis/dataio/tiff.py`:
- Around line 2419-2420: The except blocks that currently do "except Exception
as exc: raise IOError(\"Failed to inspect TIFF pyramid status for %s: %s\" %
(filename, exc))" (and the similar block at the later occurrence) are dropping
the original traceback; update both to re-raise using exception chaining: "raise
IOError(... ) from exc" so the original exception context is preserved when
raising the IOError (apply to both occurrences that reference filename and exc).
- Around line 2415-2416: The code currently only checks TIFFTAG_SUBIFD on the
current directory via tiff_file.GetField and returns “not pyramidal” too early;
instead iterate over all IFDs/directories in the TIFF (using
tiff_file.SetDirectory/NextDirectory or the library's directory iteration API)
and check TIFFTAG_SUBIFD on each one, returning true if any directory has
sub-IFDs and only returning false after all directories have been scanned;
ensure you restore the original directory (or otherwise avoid leaving the
tiff_file positioned on a different IFD) to prevent side effects.
In `@src/odemis/util/test/tiff_convert_test.py`:
- Around line 45-52: Replace the silent teardown that walks self.temp_dir and
swallows all exceptions with a deterministic remove using
shutil.rmtree(self.temp_dir) (or shutil.rmtree(self.temp_dir,
ignore_errors=True) if you intentionally want to ignore missing files), and do
not use a bare "except Exception: pass"; instead let failures surface or
explicitly handle expected OSErrors so test cleanup problems aren’t hidden.
Update the teardown that currently iterates over os.listdir/self.temp_dir to
call shutil.rmtree and reference self.temp_dir in the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b5316f0-f5fc-4c0f-8386-63522a93294a
📒 Files selected for processing (6)
src/odemis/dataio/test/tiff_test.pysrc/odemis/dataio/tiff.pysrc/odemis/gui/cont/tabs/analysis_tab.pysrc/odemis/gui/cont/tabs/correlation_tab.pysrc/odemis/util/test/tiff_convert_test.pysrc/odemis/util/tiff_convert.py
tmoerkerken
force-pushed
the
force-pyramidal-tiff-converison
branch
from
5da0453 to
e490e91
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/odemis/dataio/tiff.py (2)
2433-2434:⚠️ Potential issue | 🟠 MajorAvoid materializing the whole source TIFF during conversion.
Line 2433 loads every page into memory through
read_data()before export starts. For the large non-pyramidal TIFFs this PR is meant to handle, that can spike memory or OOM; this conversion path needs to stream pages/tiles instead of rebuilding the full dataset in RAM first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/dataio/tiff.py` around lines 2433 - 2434, The current conversion calls read_data(src_filename) which materializes every page into memory before calling export(dst_filename, data, ...); change the conversion to stream pages/tiles instead of loading the whole dataset: replace the read_data + export pattern with an iterative read-and-write loop that opens src_filename, iterates over pages/tiles (using the existing page/tile access helpers) and incrementally writes them to dst_filename (invoking export or a streaming-write helper per page/tile) while preserving compressed and pyramid flags; update code paths that reference read_data and export to use the streaming iterator so large non-pyramidal TIFFs are processed without building the full data array in RAM.
2415-2416:⚠️ Potential issue | 🟠 MajorWalk every TIFF directory before returning
False.Line 2415 only checks
TIFFTAG_SUBIFDon the current IFD. That misses pyramidal TIFFs whose first directory is a thumbnail or other non-pyramidal page, so this import path can reconvert files that are already pyramidal. Please add a regression case for a thumbnail-first pyramidal TIFF as well.🛠️ Suggested change
try: - sub_ifds = tiff_file.GetField(T.TIFFTAG_SUBIFD) - return sub_ifds is not None and len(sub_ifds) > 0 + tiff_file.SetDirectory(0) + while True: + sub_ifds = tiff_file.GetField(T.TIFFTAG_SUBIFD) + if sub_ifds: + return True + if tiff_file.LastDirectory(): + return False + tiff_file.ReadDirectory()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/dataio/tiff.py` around lines 2415 - 2416, The current check only inspects the current IFD via tiff_file.GetField(T.TIFFTAG_SUBIFD) (the sub_ifds variable) and returns False if none are found; instead iterate every directory in the TIFF (using the tiff file API to move through directories, e.g., TIFFReadDirectory()/SetDirectory() loop) and check TIFFTAG_SUBIFD on each directory, returning True if any directory has SUBIFD entries and False only after all directories were examined; update the function that contains the sub_ifds check to perform this directory-walking logic and add a regression test that constructs/reads a thumbnail-first pyramidal TIFF to ensure the path correctly detects existing pyramids.src/odemis/dataio/test/tiff_test.py (1)
28-28:⚠️ Potential issue | 🟡 MinorDon't swallow teardown failures.
Line 59 hides cleanup problems with
except Exception: pass, which makes leftover temp files hard to notice and can leak state between tests.🧹 Suggested cleanup
+import shutil import tempfile @@ def tearDown(self) -> None: - try: - for filename in os.listdir(self.temp_dir): - file_path = os.path.join(self.temp_dir, filename) - if os.path.isfile(file_path): - os.remove(file_path) - os.rmdir(self.temp_dir) - except Exception: - pass + if os.path.isdir(self.temp_dir): + shutil.rmtree(self.temp_dir)Also applies to: 58-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/dataio/test/tiff_test.py` at line 28, The test currently swallows teardown failures (the "except Exception: pass" in the teardown/cleanup block), which hides leftover temp files; change the cleanup to either use tempfile.TemporaryDirectory (or context manager) so cleanup is automatic, or catch exceptions but log and re-raise them (or use self.addCleanup) instead of silently passing; update the teardown/cleanup code (the block surrounding the tempfile usage and the "except Exception: pass") to ensure removal errors surface and are not suppressed.
🧹 Nitpick comments (2)
src/odemis/util/tiff_convert.py (1)
284-287: Closure captures loop variable by reference.The
progress_callbackclosure capturestiff_idxby reference. While the default argumentcurrent_idx=tiff_idxmitigates this, the closure definition inside the loop is repeated on each iteration, which is slightly inefficient.Consider defining the callback factory outside the loop
+ def make_progress_callback(idx: int) -> Callable[[float], None]: + def progress_callback(progress: float) -> None: + dlg.Update(int(((idx + progress) / total_tiff) * 100)) + return progress_callback + for filename in filenames: ... - def progress_callback(progress: float, current_idx: int = tiff_idx) -> None: - """Update global tileset progress (0.0 to 1.0 for current file).""" - dlg.Update(int(((current_idx + progress) / total_tiff) * 100)) + progress_callback = make_progress_callback(tiff_idx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/tiff_convert.py` around lines 284 - 287, The progress_callback closure is being redefined each loop and captures tiff_idx; refactor by creating a small factory function (e.g., make_progress_callback or create_progress_callback) outside the loop that takes an index parameter and returns the callback that uses current_idx (or sets current_idx default), then call that factory inside the loop to get the per-file callback; update references to progress_callback and current_idx/tiff_idx accordingly so the callback is not recreated inefficiently and does not rely on late-binding of tiff_idx.src/odemis/util/test/tiff_convert_test.py (1)
97-122: Consider adding a test for non-pyramidal TIFF conversion.The tests cover already-pyramidal files, non-TIFF files, and non-existent files, but there's no test verifying that a non-pyramidal TIFF actually gets converted and returns a different path. This is the core functionality of the module.
Suggested additional test
def test_ensure_pyramidal_tiff_converts_non_pyramidal(self) -> None: """Test ensure_pyramidal_tiff converts non-pyramidal file.""" non_pyramidal_file = self._create_test_tiff("non_pyramidal.tif", pyramid=False) # Call ensure_pyramidal_tiff result = ensure_pyramidal_tiff(non_pyramidal_file, standalone_mode=True) # Should return a different file (converted) self.assertNotEqual(result, non_pyramidal_file) self.assertTrue(os.path.basename(result).startswith("converted_")) self.assertTrue(os.path.isfile(result)) # Verify the converted file is pyramidal from odemis.dataio import tiff self.assertTrue(tiff.is_pyramidal(result))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/test/tiff_convert_test.py` around lines 97 - 122, Add a unit test that verifies ensure_pyramidal_tiff actually converts a non-pyramidal TIFF: create a non-pyramidal test TIFF via self._create_test_tiff("non_pyramidal.tif", pyramid=False), call ensure_pyramidal_tiff(non_pyramidal_file, standalone_mode=True), assert the returned path is not equal to the original (assertNotEqual), assert the returned file exists and its basename starts with "converted_", and finally import odemis.dataio.tiff and assert tiff.is_pyramidal(result) is True; name the test e.g. test_ensure_pyramidal_tiff_converts_non_pyramidal to match existing tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/odemis/dataio/test/tiff_test.py`:
- Line 28: The test currently swallows teardown failures (the "except Exception:
pass" in the teardown/cleanup block), which hides leftover temp files; change
the cleanup to either use tempfile.TemporaryDirectory (or context manager) so
cleanup is automatic, or catch exceptions but log and re-raise them (or use
self.addCleanup) instead of silently passing; update the teardown/cleanup code
(the block surrounding the tempfile usage and the "except Exception: pass") to
ensure removal errors surface and are not suppressed.
In `@src/odemis/dataio/tiff.py`:
- Around line 2433-2434: The current conversion calls read_data(src_filename)
which materializes every page into memory before calling export(dst_filename,
data, ...); change the conversion to stream pages/tiles instead of loading the
whole dataset: replace the read_data + export pattern with an iterative
read-and-write loop that opens src_filename, iterates over pages/tiles (using
the existing page/tile access helpers) and incrementally writes them to
dst_filename (invoking export or a streaming-write helper per page/tile) while
preserving compressed and pyramid flags; update code paths that reference
read_data and export to use the streaming iterator so large non-pyramidal TIFFs
are processed without building the full data array in RAM.
- Around line 2415-2416: The current check only inspects the current IFD via
tiff_file.GetField(T.TIFFTAG_SUBIFD) (the sub_ifds variable) and returns False
if none are found; instead iterate every directory in the TIFF (using the tiff
file API to move through directories, e.g., TIFFReadDirectory()/SetDirectory()
loop) and check TIFFTAG_SUBIFD on each directory, returning True if any
directory has SUBIFD entries and False only after all directories were examined;
update the function that contains the sub_ifds check to perform this
directory-walking logic and add a regression test that constructs/reads a
thumbnail-first pyramidal TIFF to ensure the path correctly detects existing
pyramids.
---
Nitpick comments:
In `@src/odemis/util/test/tiff_convert_test.py`:
- Around line 97-122: Add a unit test that verifies ensure_pyramidal_tiff
actually converts a non-pyramidal TIFF: create a non-pyramidal test TIFF via
self._create_test_tiff("non_pyramidal.tif", pyramid=False), call
ensure_pyramidal_tiff(non_pyramidal_file, standalone_mode=True), assert the
returned path is not equal to the original (assertNotEqual), assert the returned
file exists and its basename starts with "converted_", and finally import
odemis.dataio.tiff and assert tiff.is_pyramidal(result) is True; name the test
e.g. test_ensure_pyramidal_tiff_converts_non_pyramidal to match existing tests.
In `@src/odemis/util/tiff_convert.py`:
- Around line 284-287: The progress_callback closure is being redefined each
loop and captures tiff_idx; refactor by creating a small factory function (e.g.,
make_progress_callback or create_progress_callback) outside the loop that takes
an index parameter and returns the callback that uses current_idx (or sets
current_idx default), then call that factory inside the loop to get the per-file
callback; update references to progress_callback and current_idx/tiff_idx
accordingly so the callback is not recreated inefficiently and does not rely on
late-binding of tiff_idx.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9c5819b-5a5f-45ab-aea9-63a57af8d285
📒 Files selected for processing (6)
src/odemis/dataio/test/tiff_test.pysrc/odemis/dataio/tiff.pysrc/odemis/gui/cont/tabs/analysis_tab.pysrc/odemis/gui/cont/tabs/correlation_tab.pysrc/odemis/util/test/tiff_convert_test.pysrc/odemis/util/tiff_convert.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/odemis/gui/cont/tabs/analysis_tab.py
tmoerkerken
force-pushed
the
force-pyramidal-tiff-converison
branch
from
e490e91 to
bcccb6c
Compare
tmoerkerken
changed the title
There was a problem hiding this comment.
Pull request overview
This PR improves large-TIFF import in the Analysis/Gallery and Correlation tabs by detecting non‑pyramidal TIFFs and converting them to pyramidal TIFFs before loading, with GUI progress feedback and different output locations for Odemis vs Odemis Viewer.
Changes:
- Added
tiff.is_pyramidal()detection andtiff.convert_to_pyramidal()conversion helper inodemis.dataio.tiff. - Introduced
odemis.util.tiff_convertorchestrator + GUI wrappers (progress dialogs) and integrated them into Analysis/Correlation tab import paths. - Added unit tests for new TIFF pyramid helpers and path selection logic.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/odemis/util/tiff_convert.py |
New conversion orchestration + GUI progress wrappers; determines output location based on viewer vs project mode. |
src/odemis/util/test/tiff_convert_test.py |
New unit tests for output path logic and basic ensure_pyramidal_tiff() behavior. |
src/odemis/gui/cont/tabs/correlation_tab.py |
Enforces pyramidal TIFF conversion on single-file and tileset import. |
src/odemis/gui/cont/tabs/analysis_tab.py |
Enforces pyramidal TIFF conversion on single-file and tileset import. |
src/odemis/dataio/tiff.py |
Adds pyramid detection and conversion API functions. |
src/odemis/dataio/test/tiff_test.py |
Adds tests for is_pyramidal() and convert_to_pyramidal(). |
You can also share your feedback on Copilot code review. Take the survey.
src/odemis/util/tiff_convert.py
Outdated
Comment on lines
+240
to
+253
| tiff_files = [fn for fn in filenames if fn.lower().endswith((".tif", ".tiff"))] | ||
| total_tiff = len(tiff_files) | ||
| if total_tiff == 0: | ||
| return filenames | ||
|
|
||
| standalone, project_folder = _get_storage_context(main_data) | ||
| dlg = wx.ProgressDialog( | ||
| "Converting TIFF Tileset", | ||
| f"Converting tileset files (0/{total_tiff}) to pyramidal format...", | ||
| maximum=100, | ||
| parent=parent, | ||
| style=wx.PD_APP_MODAL | wx.PD_AUTO_HIDE, | ||
| ) | ||
|
|
src/odemis/dataio/tiff.py
Outdated
Comment on lines
+2423
to
+2435
| def convert_to_pyramidal(src_filename: str, dst_filename: str, compressed: bool = True) -> None: | ||
| """ | ||
| Convert TIFF content into a pyramidal TIFF. | ||
|
|
||
| :param src_filename: Source TIFF file path | ||
| :param dst_filename: Destination TIFF file path | ||
| :param compressed: Whether to write with TIFF compression | ||
| :raises IOError: If conversion fails | ||
| """ | ||
| try: | ||
| data = read_data(src_filename) | ||
| export(dst_filename, data, compressed=compressed, pyramid=True) | ||
| except Exception as exc: |
Comment on lines
+97
to
+122
| def test_ensure_pyramidal_tiff_already_pyramidal(self) -> None: | ||
| """Test ensure_pyramidal_tiff with already pyramidal file.""" | ||
| pyramidal_file = self._create_test_tiff("already_pyramidal.tif", pyramid=True) | ||
|
|
||
| # Call ensure_pyramidal_tiff | ||
| result = ensure_pyramidal_tiff(pyramidal_file, standalone_mode=True) | ||
|
|
||
| # Should return the same file without conversion | ||
| self.assertEqual(result, pyramidal_file) | ||
|
|
||
| def test_ensure_pyramidal_tiff_non_tiff_file(self) -> None: | ||
| """Test ensure_pyramidal_tiff with non-TIFF file.""" | ||
| # Create a non-TIFF file | ||
| non_tiff_file = os.path.join(self.temp_dir, "test.txt") | ||
| with open(non_tiff_file, 'w') as f: | ||
| f.write("test content") | ||
|
|
||
| # Non-TIFF should return as-is (in ensure_pyramidal_tiff before conversion attempt) | ||
| result = ensure_pyramidal_tiff(non_tiff_file, standalone_mode=True) | ||
| self.assertEqual(result, non_tiff_file) | ||
|
|
||
| def test_ensure_pyramidal_tiff_nonexistent_file(self) -> None: | ||
| """Test ensure_pyramidal_tiff with non-existent file.""" | ||
| with self.assertRaises(IOError): | ||
| ensure_pyramidal_tiff("/nonexistent/file.tif", standalone_mode=True) | ||
|
|
src/odemis/util/tiff_convert.py
Outdated
| import os | ||
| from typing import Any, Callable, Optional | ||
|
|
||
| import wx |
Member
There was a problem hiding this comment.
@tmoerkerken I agree here that if this module relies on wx then it should not be here. I'd suggest to move it to odemis.gui.util. The alternative would be to have a more indirect way to report the progress: run the function in background, and return a ProgressiveFuture. Then, in the GUI side, create a progress dialog, and connect the ProgressiveFuture to the progress bar.
src/odemis/util/tiff_convert.py
Outdated
Comment on lines
+62
to
+100
| def convert_to_pyramidal_with_progress( | ||
| src_filename: str, | ||
| dst_filename: str, | ||
| compression: str = "lzw", | ||
| progress_callback: Optional[Callable[[float], None]] = None, | ||
| ) -> None: | ||
| """ | ||
| Convert a non-pyramidal TIFF file to pyramidal format. | ||
|
|
||
| Uses libtiff and the dataio module's export function with pyramid=True. | ||
|
|
||
| :param src_filename: Path to source non-pyramidal TIFF file | ||
| :param dst_filename: Path where converted pyramidal TIFF will be saved | ||
| :param compression: Compression type ('lzw', 'lz4', 'zstd', None for uncompressed) | ||
| :param progress_callback: Optional callable to report progress (0.0 to 1.0) | ||
| :raises IOError: If conversion fails | ||
| :raises ValueError: If source file doesn't exist or is not a valid TIFF | ||
| """ | ||
| if not os.path.isfile(src_filename): | ||
| raise ValueError(f"Source file not found: {src_filename}") | ||
|
|
||
| try: | ||
| dst_dir = os.path.dirname(dst_filename) | ||
| if progress_callback: | ||
| progress_callback(0.1) | ||
|
|
||
| if dst_dir: | ||
| os.makedirs(dst_dir, exist_ok=True) | ||
|
|
||
| if progress_callback: | ||
| progress_callback(0.5) | ||
|
|
||
| logger.info("Converting to pyramidal format and saving to: %s", dst_filename) | ||
|
|
||
| # Determine if we need to compress | ||
| compressed = compression is not None and compression.lower() != "none" | ||
|
|
||
| tiff.convert_to_pyramidal(src_filename, dst_filename, compressed=compressed) | ||
|
|
src/odemis/util/tiff_convert.py
Outdated
Comment on lines
+155
to
+160
| target_project_folder = None if standalone_mode else project_folder | ||
| output_path = get_conversion_output_path(filename, target_project_folder) | ||
|
|
||
| # If converted file already exists, we'll overwrite it | ||
| # (no caching per requirements) | ||
| convert_to_pyramidal_with_progress(filename, output_path, progress_callback=progress_callback) |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
src/odemis/dataio/test/tiff_test.py (2)
58-66:⚠️ Potential issue | 🟡 MinorAvoid swallowing teardown failures silently.
Line 65 catches all exceptions and discards them, which can mask cleanup bugs and leave residual files between tests.
🧹 Proposed fix
+import shutil @@ def tearDown(self) -> None: - try: - for filename in os.listdir(self.temp_dir): - file_path = os.path.join(self.temp_dir, filename) - if os.path.isfile(file_path): - os.remove(file_path) - os.rmdir(self.temp_dir) - except Exception: - pass + shutil.rmtree(self.temp_dir, ignore_errors=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/dataio/test/tiff_test.py` around lines 58 - 66, The tearDown method currently swallows all exceptions (catch-all in tearDown) which hides cleanup failures; update tearDown in tiff_test.py to avoid silently discarding errors by removing the bare except: pass. Instead, perform deterministic cleanup (use shutil.rmtree(self.temp_dir) or loop with os.remove/os.rmdir) and if an exception occurs either log it or re-raise it so tests fail visibly; reference the tearDown method and self.temp_dir when making the change and ensure any temporary-residue is removed reliably.
53-95:⚠️ Potential issue | 🟡 MinorAdd docstrings for the new test class and methods.
TestTiffPyramidalIOand its new methods are missing docstrings.📝 Proposed fix
class TestTiffPyramidalIO(unittest.TestCase): + """Tests pyramidal TIFF detection and conversion helpers.""" @@ def setUp(self) -> None: + """Create a temporary workspace for TIFF test files.""" self.temp_dir = tempfile.mkdtemp() @@ def tearDown(self) -> None: + """Remove temporary files created by the test case.""" @@ def _create_test_tiff(self, filename: str, pyramid: bool = False) -> str: + """Create and export a test TIFF file in the temporary directory.""" @@ def test_is_pyramidal_tiff(self) -> None: + """Verify pyramidal TIFFs are detected as pyramidal.""" @@ def test_is_pyramidal_non_pyramidal_tiff(self) -> None: + """Verify non-pyramidal TIFFs are detected correctly.""" @@ def test_is_pyramidal_invalid_file(self) -> None: + """Verify invalid paths raise IOError in pyramid detection.""" @@ def test_convert_to_pyramidal_tiff(self) -> None: + """Verify conversion creates a pyramidal TIFF output file."""As per coding guidelines: "Include docstrings for all functions and classes, following the reStructuredText style guide (without type information)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/dataio/test/tiff_test.py` around lines 53 - 95, Add reStructuredText-style docstrings (without type info) to the TestTiffPyramidalIO test class and each method: setUp, tearDown, _create_test_tiff, test_is_pyramidal_tiff, test_is_pyramidal_non_pyramidal_tiff, test_is_pyramidal_invalid_file, and test_convert_to_pyramidal_tiff; each docstring should briefly describe the purpose of the class/method and any important behavior (e.g., setUp creates temp dir, _create_test_tiff builds a test TIFF with optional pyramid, test_* methods assert pyramidal detection and conversion behavior).src/odemis/dataio/tiff.py (1)
2419-2420:⚠️ Potential issue | 🟡 MinorChain wrapped
IOErrorexceptions to preserve root cause.Line 2420 and Line 2437 re-raise without
from exc, which drops the original traceback and makes conversion/read failures harder to diagnose.🔧 Proposed fix
- except Exception as exc: - raise IOError("Failed to inspect TIFF pyramid status for %s: %s" % (filename, exc)) + except Exception as exc: + raise IOError("Failed to inspect TIFF pyramid status for %s: %s" % (filename, exc)) from exc @@ - except Exception as exc: - raise IOError("Failed to convert TIFF %s to pyramidal %s: %s" % - (src_filename, dst_filename, exc)) + except Exception as exc: + raise IOError("Failed to convert TIFF %s to pyramidal %s: %s" % + (src_filename, dst_filename, exc)) from excAlso applies to: 2435-2437
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/dataio/tiff.py` around lines 2419 - 2420, The except blocks that currently do "except Exception as exc: raise IOError(... % (filename, exc))" must re-raise with exception chaining to preserve the original traceback; update both occurrences (the except at the inspection block around line ~2420 and the similar block around lines ~2435-2437) to use "raise IOError(<message>) from exc" (you can still include filename in the message) so the original exception is preserved and traceable.src/odemis/util/tiff_convert.py (2)
106-114:⚠️ Potential issue | 🟡 MinorPreserve root traceback when wrapping conversion errors.
Line 114 and Line 169 re-raise new
IOErrors without chaining, which drops original exception context.Proposed fix
- except Exception as e: + except Exception as e: logger.error(f"Error converting TIFF to pyramidal format: {e}") # Clean up partial file if it exists if os.path.exists(dst_filename): try: os.remove(dst_filename) except Exception as cleanup_error: logger.warning(f"Failed to clean up partial file {dst_filename}: {cleanup_error}") - raise IOError(f"Failed to convert TIFF file: {e}") + raise IOError(f"Failed to convert TIFF file: {e}") from e @@ - except Exception as e: + except Exception as e: logger.error(f"Unexpected error in ensure_pyramidal_tiff: {e}") - raise IOError(f"Unexpected error processing TIFF file: {e}") + raise IOError(f"Unexpected error processing TIFF file: {e}") from eAlso applies to: 167-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/tiff_convert.py` around lines 106 - 114, The current except blocks log the error and then raise new IOError instances without chaining, which loses the original traceback; update both handlers (the block that references dst_filename, logger.error and the later handler around lines 167-169) to re-raise using exception chaining (e.g., raise IOError(f"Failed to convert TIFF file: {e}") from e) so the original exception context is preserved while keeping the existing cleanup logic that removes dst_filename and logs cleanup warnings.
265-274:⚠️ Potential issue | 🟠 MajorUse one conversion path in the tileset loop to avoid uncaught inspection failures.
Line 265-Line 274 manually calls
tiff.is_pyramidaland only catchesIOErrorthere. If a different exception type is raised, the loop can abort instead of falling back to the original file.Proposed refactor
- try: - if tiff.is_pyramidal(filename): - converted_files.append(filename) - dlg.Update(int(((tiff_idx + 1) / total_tiff) * 100)) - continue - except IOError as exc: - logger.error("Failed to inspect TIFF file %s: %s", filename, exc) - converted_files.append(filename) - dlg.Update(int(((tiff_idx + 1) / total_tiff) * 100)) - continue - dlg.Update( int((tiff_idx / total_tiff) * 100), f"Converting tileset files ({tiff_idx + 1}/{total_tiff}) to pyramidal format...", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/tiff_convert.py` around lines 265 - 274, The tiff inspection call tiff.is_pyramidal(...) only catches IOError and on exception incorrectly treats the file as already converted; change the tileset loop so the is_pyramidal call is wrapped with a broad except Exception, and on any exception log the error (using logger.error) but do NOT append filename to converted_files or skip conversion—allow the normal non-pyramidal conversion path to run (so converted_files, dlg.Update, tiff_idx and total_tiff handling remain correct); this ensures any inspection failure falls back to conversion instead of aborting or incorrectly marking the file converted.src/odemis/util/test/tiff_convert_test.py (2)
45-52:⚠️ Potential issue | 🟡 MinorDo not swallow teardown cleanup errors.
Line 51 catches
Exceptionand silently ignores failures, which can hide filesystem/test pollution issues.Proposed fix
import os +import shutil import tempfile import unittest import numpy @@ def tearDown(self) -> None: """Clean up temporary files.""" - # Remove all temporary files - try: - for filename in os.listdir(self.temp_dir): - file_path = os.path.join(self.temp_dir, filename) - if os.path.isfile(file_path): - os.remove(file_path) - os.rmdir(self.temp_dir) - except Exception: - pass + if os.path.isdir(self.temp_dir): + shutil.rmtree(self.temp_dir)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/test/tiff_convert_test.py` around lines 45 - 52, The teardown currently swallows all exceptions (catching Exception) when cleaning up self.temp_dir; change this to not hide failures by either removing the broad try/except so errors from os.listdir/os.remove/os.rmdir propagate to the test runner, or catch only expected exceptions (e.g., OSError) and log them before re-raising; specifically update the block around os.listdir(self.temp_dir), os.remove(file_path) and os.rmdir(self.temp_dir) so you do not silently ignore Exception and instead let the failure surface (or log with logger.exception and re-raise) so filesystem/test pollution issues are visible.
36-36:⚠️ Potential issue | 🟡 MinorAdd a class docstring for
TestTiffConversion.Line 36 defines the class without a docstring.
Proposed fix
class TestTiffConversion(unittest.TestCase): + """Tests TIFF conversion helpers and pyramidal-enforcement behavior."""As per coding guidelines: "Include docstrings for all functions and classes, following the reStructuredText style guide (without type information)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/test/tiff_convert_test.py` at line 36, Add a reStructuredText-style docstring to the TestTiffConversion class describing its purpose (e.g., "Unit tests for TIFF conversion utilities"), placement immediately below the class definition for TestTiffConversion, and keep it concise (one or two sentences) without type information; ensure it follows project docstring conventions used by other test classes.src/odemis/gui/cont/tabs/analysis_tab.py (1)
325-335:⚠️ Potential issue | 🟠 MajorAdd type hints and docstrings to the touched loader methods.
Line 325 (
load_tileset) and Line 331 (load_data) are changed but still missing parameter/return annotations and function docstrings.Proposed fix
- def load_tileset(self, filenames, extend=False): + def load_tileset(self, filenames: list[str], extend: bool = False) -> None: + """Load and stitch files after ensuring TIFF inputs are pyramidal.""" # Convert all files to pyramidal if they're TIFFs filenames = ensure_pyramidal_tiff_for_tileset_gui(filenames, self.panel, self.tab_data_model.main) data = open_files_and_stitch(filenames) # TODO: allow user defined registration / weave methods self.display_new_data(filenames[0], data, extend=extend) - def load_data(self, filename, fmt=None, extend=False): + def load_data(self, filename: str, fmt: str | None = None, extend: bool = False) -> None: + """Load one file after ensuring TIFF input is pyramidal.""" # Convert to pyramidal if it's a TIFF file filename = ensure_pyramidal_tiff_for_file_gui(filename, self.panel, self.tab_data_model.main) data = open_acquisition(filename, fmt) self.display_new_data(filename, data, extend=extend)As per coding guidelines: "Always use type hints for function parameters and return types in Python code" and "Include docstrings for all functions and classes, following the reStructuredText style guide (without type information)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/tabs/analysis_tab.py` around lines 325 - 335, Add proper type hints and reStructuredText-style docstrings to the two loader methods: annotate load_tileset(self, filenames, extend=False) -> None (e.g., filenames: Sequence[str], extend: bool = False) and load_data(self, filename, fmt=None, extend=False) -> None (e.g., filename: str, fmt: Optional[str], extend: bool = False). In each docstring describe purpose, parameters and return value (no type info in the docstring) and reference the behavior that these call ensure_pyramidal_tiff_for_tileset_gui / ensure_pyramidal_tiff_for_file_gui, call open_files_and_stitch / open_acquisition, and then call display_new_data; ensure the annotations match how those functions are used and the functions return None.
🧹 Nitpick comments (2)
src/odemis/gui/cont/tabs/correlation_tab.py (1)
179-179: Use explicit optional typing forfmt.
fmt: str = Noneshould be typed as optional (Optional[str]orstr | None) for PEP 484-compliant annotations.🔎 Proposed fix
-def load_data(self, filename: str, fmt: str = None, extend: bool = False) -> None: +def load_data(self, filename: str, fmt: Optional[str] = None, extend: bool = False) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/tabs/correlation_tab.py` at line 179, The parameter annotation for load_data uses fmt: str = None which is not PEP 484-compliant; update the signature of load_data to use an explicit optional type (e.g., fmt: Optional[str] = None or fmt: str | None = None) and add the corresponding import (from typing import Optional) if using Optional so the function signature clearly indicates that fmt may be None; ensure references to load_data remain unchanged otherwise.src/odemis/util/test/tiff_convert_test.py (1)
97-122: Add a happy-path test for actual non-pyramidal TIFF conversion.Current tests cover already-pyramidal, non-TIFF, and missing-file paths, but not the main conversion-success path.
Suggested test case
+ def test_ensure_pyramidal_tiff_converts_non_pyramidal(self) -> None: + """Test ensure_pyramidal_tiff converts a non-pyramidal TIFF.""" + source = self._create_test_tiff("non_pyramidal.tif", pyramid=False) + output = ensure_pyramidal_tiff(source, standalone_mode=True) + + self.assertNotEqual(output, source) + self.assertTrue(os.path.isfile(output)) + self.assertTrue(tiff.is_pyramidal(output))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/test/tiff_convert_test.py` around lines 97 - 122, Add a happy-path test that creates a non-pyramidal TIFF (use the existing helper _create_test_tiff with pyramid=False), calls ensure_pyramidal_tiff on it, and asserts conversion succeeded by checking the returned path is different from the original, the returned file exists, and the returned TIFF is pyramidal (inspect via the same TIFF-inspection helper or by asserting multiple image levels/directories). Name the test e.g. test_ensure_pyramidal_tiff_converts_non_pyramidal and use ensure_pyramidal_tiff and _create_test_tiff to locate the code under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/gui/cont/tabs/correlation_tab.py`:
- Around line 173-183: Add reStructuredText-style docstrings (no type
annotations) to the load_tileset and load_data methods describing purpose,
parameters, and behavior; for load_tileset explain it converts TIFFs to
pyramidal, stitches files into a data object and calls load_streams (mention
parameters filenames and optional extend and any GUI interactions via
self.panel/self.main_data), and for load_data explain it converts a TIFF to
pyramidal, opens an acquisition with open_acquisition and calls load_streams
(mention parameters filename, optional fmt and extend). Keep them brief, in rst
summary + :param: lines, and place them immediately under the def lines for
load_tileset and load_data.
- Around line 173-183: Both load_tileset and load_data call
ensure_pyramidal_tiff_for_tileset_gui / ensure_pyramidal_tiff_for_file_gui (and
subsequent open_* functions) without handling failures, so conversion/load
errors can abort the UI flow; wrap the calls to
ensure_pyramidal_tiff_for_tileset_gui, open_files_and_stitch (in load_tileset)
and ensure_pyramidal_tiff_for_file_gui, open_acquisition (in load_data) in
try/except, catch any exceptions, log the exception, show a user-facing error
via the tab panel (e.g., self.panel.show_error or similar), and return early to
avoid calling self.load_streams when an error occurs; keep the existing
signatures of load_tileset and load_data and ensure the error message includes
the exception text for debugging.
---
Duplicate comments:
In `@src/odemis/dataio/test/tiff_test.py`:
- Around line 58-66: The tearDown method currently swallows all exceptions
(catch-all in tearDown) which hides cleanup failures; update tearDown in
tiff_test.py to avoid silently discarding errors by removing the bare except:
pass. Instead, perform deterministic cleanup (use shutil.rmtree(self.temp_dir)
or loop with os.remove/os.rmdir) and if an exception occurs either log it or
re-raise it so tests fail visibly; reference the tearDown method and
self.temp_dir when making the change and ensure any temporary-residue is removed
reliably.
- Around line 53-95: Add reStructuredText-style docstrings (without type info)
to the TestTiffPyramidalIO test class and each method: setUp, tearDown,
_create_test_tiff, test_is_pyramidal_tiff, test_is_pyramidal_non_pyramidal_tiff,
test_is_pyramidal_invalid_file, and test_convert_to_pyramidal_tiff; each
docstring should briefly describe the purpose of the class/method and any
important behavior (e.g., setUp creates temp dir, _create_test_tiff builds a
test TIFF with optional pyramid, test_* methods assert pyramidal detection and
conversion behavior).
In `@src/odemis/dataio/tiff.py`:
- Around line 2419-2420: The except blocks that currently do "except Exception
as exc: raise IOError(... % (filename, exc))" must re-raise with exception
chaining to preserve the original traceback; update both occurrences (the except
at the inspection block around line ~2420 and the similar block around lines
~2435-2437) to use "raise IOError(<message>) from exc" (you can still include
filename in the message) so the original exception is preserved and traceable.
In `@src/odemis/gui/cont/tabs/analysis_tab.py`:
- Around line 325-335: Add proper type hints and reStructuredText-style
docstrings to the two loader methods: annotate load_tileset(self, filenames,
extend=False) -> None (e.g., filenames: Sequence[str], extend: bool = False) and
load_data(self, filename, fmt=None, extend=False) -> None (e.g., filename: str,
fmt: Optional[str], extend: bool = False). In each docstring describe purpose,
parameters and return value (no type info in the docstring) and reference the
behavior that these call ensure_pyramidal_tiff_for_tileset_gui /
ensure_pyramidal_tiff_for_file_gui, call open_files_and_stitch /
open_acquisition, and then call display_new_data; ensure the annotations match
how those functions are used and the functions return None.
In `@src/odemis/util/test/tiff_convert_test.py`:
- Around line 45-52: The teardown currently swallows all exceptions (catching
Exception) when cleaning up self.temp_dir; change this to not hide failures by
either removing the broad try/except so errors from
os.listdir/os.remove/os.rmdir propagate to the test runner, or catch only
expected exceptions (e.g., OSError) and log them before re-raising; specifically
update the block around os.listdir(self.temp_dir), os.remove(file_path) and
os.rmdir(self.temp_dir) so you do not silently ignore Exception and instead let
the failure surface (or log with logger.exception and re-raise) so
filesystem/test pollution issues are visible.
- Line 36: Add a reStructuredText-style docstring to the TestTiffConversion
class describing its purpose (e.g., "Unit tests for TIFF conversion utilities"),
placement immediately below the class definition for TestTiffConversion, and
keep it concise (one or two sentences) without type information; ensure it
follows project docstring conventions used by other test classes.
In `@src/odemis/util/tiff_convert.py`:
- Around line 106-114: The current except blocks log the error and then raise
new IOError instances without chaining, which loses the original traceback;
update both handlers (the block that references dst_filename, logger.error and
the later handler around lines 167-169) to re-raise using exception chaining
(e.g., raise IOError(f"Failed to convert TIFF file: {e}") from e) so the
original exception context is preserved while keeping the existing cleanup logic
that removes dst_filename and logs cleanup warnings.
- Around line 265-274: The tiff inspection call tiff.is_pyramidal(...) only
catches IOError and on exception incorrectly treats the file as already
converted; change the tileset loop so the is_pyramidal call is wrapped with a
broad except Exception, and on any exception log the error (using logger.error)
but do NOT append filename to converted_files or skip conversion—allow the
normal non-pyramidal conversion path to run (so converted_files, dlg.Update,
tiff_idx and total_tiff handling remain correct); this ensures any inspection
failure falls back to conversion instead of aborting or incorrectly marking the
file converted.
---
Nitpick comments:
In `@src/odemis/gui/cont/tabs/correlation_tab.py`:
- Line 179: The parameter annotation for load_data uses fmt: str = None which is
not PEP 484-compliant; update the signature of load_data to use an explicit
optional type (e.g., fmt: Optional[str] = None or fmt: str | None = None) and
add the corresponding import (from typing import Optional) if using Optional so
the function signature clearly indicates that fmt may be None; ensure references
to load_data remain unchanged otherwise.
In `@src/odemis/util/test/tiff_convert_test.py`:
- Around line 97-122: Add a happy-path test that creates a non-pyramidal TIFF
(use the existing helper _create_test_tiff with pyramid=False), calls
ensure_pyramidal_tiff on it, and asserts conversion succeeded by checking the
returned path is different from the original, the returned file exists, and the
returned TIFF is pyramidal (inspect via the same TIFF-inspection helper or by
asserting multiple image levels/directories). Name the test e.g.
test_ensure_pyramidal_tiff_converts_non_pyramidal and use ensure_pyramidal_tiff
and _create_test_tiff to locate the code under test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1145507-fa5e-4d3c-9ec6-2c1d8c4fb1fd
📒 Files selected for processing (6)
src/odemis/dataio/test/tiff_test.pysrc/odemis/dataio/tiff.pysrc/odemis/gui/cont/tabs/analysis_tab.pysrc/odemis/gui/cont/tabs/correlation_tab.pysrc/odemis/util/test/tiff_convert_test.pysrc/odemis/util/tiff_convert.py
Comment on lines
173
to
183
| def load_tileset(self, filenames: List[str], extend: bool = False) -> None: | ||
| # Convert all files to pyramidal if they're TIFFs | ||
| filenames = ensure_pyramidal_tiff_for_tileset_gui(filenames, self.panel, self.main_data) | ||
| data = open_files_and_stitch(filenames) # TODO: allow user defined registration / weave methods | ||
| self.load_streams(data) | ||
|
|
||
| def load_data(self, filename: str, fmt: str = None, extend: bool = False) -> None: | ||
| # Convert to pyramidal if it's a TIFF file | ||
| filename = ensure_pyramidal_tiff_for_file_gui(filename, self.panel, self.main_data) | ||
| data = open_acquisition(filename, fmt) | ||
| self.load_streams(data) |
There was a problem hiding this comment.
Add docstrings for modified loader methods.
load_tileset and load_data were updated but still have no docstrings.
As per coding guidelines: "Include docstrings for all functions and classes, following the reStructuredText style guide (without type information)".
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 179-179: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/gui/cont/tabs/correlation_tab.py` around lines 173 - 183, Add
reStructuredText-style docstrings (no type annotations) to the load_tileset and
load_data methods describing purpose, parameters, and behavior; for load_tileset
explain it converts TIFFs to pyramidal, stitches files into a data object and
calls load_streams (mention parameters filenames and optional extend and any GUI
interactions via self.panel/self.main_data), and for load_data explain it
converts a TIFF to pyramidal, opens an acquisition with open_acquisition and
calls load_streams (mention parameters filename, optional fmt and extend). Keep
them brief, in rst summary + :param: lines, and place them immediately under the
def lines for load_tileset and load_data.
Handle TIFF conversion/load failures with user-facing error flow.
ensure_pyramidal_tiff_for_tileset_gui and ensure_pyramidal_tiff_for_file_gui can fail; without local handling here, a conversion error can abort the tab action path abruptly.
🛡️ Proposed fix
def load_tileset(self, filenames: List[str], extend: bool = False) -> None:
- # Convert all files to pyramidal if they're TIFFs
- filenames = ensure_pyramidal_tiff_for_tileset_gui(filenames, self.panel, self.main_data)
- data = open_files_and_stitch(filenames) # TODO: allow user defined registration / weave methods
- self.load_streams(data)
+ try:
+ # Convert all files to pyramidal if they're TIFFs
+ filenames = ensure_pyramidal_tiff_for_tileset_gui(filenames, self.panel, self.main_data)
+ data = open_files_and_stitch(filenames) # TODO: allow user defined registration / weave methods
+ except IOError as exc:
+ logging.exception("Failed to load tileset after TIFF conversion: %s", exc)
+ wx.MessageBox(str(exc), "Failed to load tileset", wx.ICON_ERROR | wx.OK, parent=self.panel)
+ return
+ self.load_streams(data)
@@
def load_data(self, filename: str, fmt: str = None, extend: bool = False) -> None:
- # Convert to pyramidal if it's a TIFF file
- filename = ensure_pyramidal_tiff_for_file_gui(filename, self.panel, self.main_data)
- data = open_acquisition(filename, fmt)
- self.load_streams(data)
+ try:
+ # Convert to pyramidal if it's a TIFF file
+ filename = ensure_pyramidal_tiff_for_file_gui(filename, self.panel, self.main_data)
+ data = open_acquisition(filename, fmt)
+ except IOError as exc:
+ logging.exception("Failed to load file after TIFF conversion: %s", exc)
+ wx.MessageBox(str(exc), "Failed to load file", wx.ICON_ERROR | wx.OK, parent=self.panel)
+ return
+ self.load_streams(data)🧰 Tools
🪛 Ruff (0.15.6)
[warning] 179-179: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/gui/cont/tabs/correlation_tab.py` around lines 173 - 183, Both
load_tileset and load_data call ensure_pyramidal_tiff_for_tileset_gui /
ensure_pyramidal_tiff_for_file_gui (and subsequent open_* functions) without
handling failures, so conversion/load errors can abort the UI flow; wrap the
calls to ensure_pyramidal_tiff_for_tileset_gui, open_files_and_stitch (in
load_tileset) and ensure_pyramidal_tiff_for_file_gui, open_acquisition (in
load_data) in try/except, catch any exceptions, log the exception, show a
user-facing error via the tab panel (e.g., self.panel.show_error or similar),
and return early to avoid calling self.load_streams when an error occurs; keep
the existing signatures of load_tileset and load_data and ensure the error
message includes the exception text for debugging.
tepals
reviewed
Mar 18, 2026
pieleric
requested changes
Mar 18, 2026
src/odemis/util/tiff_convert.py
Outdated
| import os | ||
| from typing import Any, Callable, Optional | ||
|
|
||
| import wx |
Member
There was a problem hiding this comment.
@tmoerkerken I agree here that if this module relies on wx then it should not be here. I'd suggest to move it to odemis.gui.util. The alternative would be to have a more indirect way to report the progress: run the function in background, and return a ProgressiveFuture. Then, in the GUI side, create a progress dialog, and connect the ProgressiveFuture to the progress bar.
src/odemis/dataio/tiff.py
Outdated
| raise IOError("Failed to inspect TIFF pyramid status for %s: %s" % (filename, exc)) | ||
|
|
||
|
|
||
| def convert_to_pyramidal(src_filename: str, dst_filename: str, compressed: bool = True) -> None: |
Member
There was a problem hiding this comment.
I don't think it should be just in "TIFF". We can make it generic, although for now only TIFF supports pyramidal storage. Let's move this function to odemis.util.dataio . It should call .read_data for the given file, independent of whether the input file format. To export, of course, if the exporter doesn't support pyramidal format, this will fail (but you can check early, by using .CAN_SAVE_PYRAMID flag)
|
|
||
| def load_tileset(self, filenames, extend=False): | ||
| # Convert all files to pyramidal if they're TIFFs | ||
| filenames = ensure_pyramidal_tiff_for_tileset_gui(filenames, self.panel, self.tab_data_model.main) |
Member
There was a problem hiding this comment.
No. That's missing the point.
The idea is that the user selects a series of tiles, and they are first weaved into a single large image by open_files_and_stitch(), and afterwards this large single image, instead of being kept in memory should be stored to a pyramidal TIFF file, and reopened.
So the storage to pyramidal file should happen after open_files_and_stitch().
You can find example data here: https://drive.google.com/drive/folders/1itNGSYq4bQJBZIZDKLYLMtRdFpzswNye
(Though, currently not properly displayed due to the scan rotation, but I have a PR for that)
tmoerkerken
force-pushed
the
force-pyramidal-tiff-converison
branch
from
bcccb6c to
83e48aa
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
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.
This PR improves TIFF import for Correlation/Gallery tabs when using Add stream (From file / From tileset).
When importing large non-pyramidal TIFFs, Odemis now detects this and converts them to pyramidal TIFF first, then loads the converted file.
Changes
Demo
Opening single file and tileset
tiff.webm
Reopening converted file
reopening_converted_tiff.webm
Warning
I removed the progress bar since the recording, since the conversion is typically so quick that it is not necessary at this point.