Skip to content

Conversation

@lguerard
Copy link
Contributor

@lguerard lguerard commented Jan 14, 2026

This pull request introduces several enhancements and refactorings to the src/imcflibs/imagej/bdv.py module, focusing on improving dataset definition and export workflows, as well as increasing flexibility and maintainability. The most significant updates include support for directly passing file lists to the manual dataset definition, improved options for exporting datasets to OME-TIFF, and the addition of a utility for channel-suffixed file naming.

Enhancements to dataset definition and export:

  • Added support for passing a list of files (list_files) directly to the define_dataset_manual function, enabling the use of explicit file lists in "show_list" mode rather than relying solely on file patterns. The options string is now constructed accordingly. [1] [2] [3]
  • Refactored the fuse_dataset_bdvp function to allow specifying the number of resolution levels, LZW compression, and the fusion method, and updated its parameter names and documentation for clarity and flexibility. The function now also waits for the export command to complete.

Utility and code quality improvements:

  • Added a new utility function join_files_with_channel_suffix to generate lists of filenames with channel suffixes, facilitating workflows where each channel is stored in a separate file.
  • Updated the resave_as_h5 function to use the correct processing options object and changed the ImageJ command to "Resave as HDF5 (local)" for better accuracy. [1] [2]

Imports and minor cleanups:

  • Added import for AlphaFusedResampledSource to support the new fusion method default, and made minor whitespace and code cleanups. [1] [2]

* Add optional list_files parameter to define_dataset_manual
* Update processing options to use bdv.ProcessingOptions
* Improve logging for manual dataset definition options
* Refactor fuse_dataset_bdvp parameters for clarity
* Introduce join_files_with_channel_suffix function for file handling
@lguerard lguerard changed the base branch from master to devel January 14, 2026 08:32
@ehrenfeu ehrenfeu added blocked Blocked by another issue / PR new-major Change introduced in PR will require a new major release. labels Jan 14, 2026
@ehrenfeu ehrenfeu added this to the 1.6.0 milestone Jan 14, 2026
Copy link
Member

@ehrenfeu ehrenfeu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, please remove the changes to fuse_dataset_bdvp from this PR and merge them into #106.

@ehrenfeu ehrenfeu added the duplicate This issue or pull request already exists label Jan 14, 2026
@ehrenfeu ehrenfeu moved this to In progress in imcflibs Jan 14, 2026
@CellKai CellKai mentioned this pull request Jan 14, 2026
@codecov
Copy link

codecov bot commented Jan 14, 2026

Codecov Report

❌ Patch coverage is 4.54545% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 24%. Comparing base (745fc61) to head (a03a62e).
⚠️ Report is 51 commits behind head on devel.

Files with missing lines Patch % Lines
src/imcflibs/imagej/bdv.py 5% 21 Missing ⚠️
Additional details and impacted files
@@         Coverage Diff          @@
##           devel   #118   +/-   ##
====================================
- Coverage     25%    24%   -1%     
====================================
  Files         25     25           
  Lines       1688   1733   +45     
====================================
+ Hits         421    423    +2     
- Misses      1267   1310   +43     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ehrenfeu ehrenfeu added bug Something isn't working and removed duplicate This issue or pull request already exists blocked Blocked by another issue / PR labels Jan 15, 2026
Use the Python function parameter name ('source_directory')
instead of the parameter name used by the IJ macro.
Copy link
Member

@ehrenfeu ehrenfeu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 Please do a git pull before, as I made a few changes to the PR already! 🚨

I added several comments to be addressed here before we can merge this.


if definition_opts is None:
definition_opts = DefinitionOptions()
definition_opts = bdv.DefinitionOptions()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is expected to fail as DefinitionOptions is a class defined in this very file here.

➡️ Nicely illustrates the need for unit tests.


if not processing_opts:
processing_opts = ProcessingOptions()
processing_opts = bdv.ProcessingOptions()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Examples
--------
fuse_dataset_bdvp(xml_input, cs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example doesn't provide any additional information as it is just the most simple case of how to call this function.

Possible options:

  • either remove the example or
  • provide additional value by adding code that shows at least how to construct the two parameters used here
  • if possible also have a more complex example that uses more of the function's parameters

Note: please use the right syntax for docstring examples (they need to be prefixed with ">>> ") in order to have them rendered correctly, see the numpydoc format documentation, section "Examples" for details.

).get()


def join_files_with_channel_suffix(files, nchannels):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several requests here:

  • Please move this into imcflibs.pathtools as this is not BDV related and potentially useful for other parts as well.
  • Any import command should be at the module top, unless there is a very good reason.
  • Please fix the docstring syntax to use single backticks instead of doubled backticks: https://pdoc.dev/docs/pdoc.html#link-to-other-identifiers

@ehrenfeu ehrenfeu added next-release Issues blocking the next release conventions Issue with formatting / naming / ... changelog Needs to be mentioned in release changelogs and removed bug Something isn't working labels Jan 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog Needs to be mentioned in release changelogs conventions Issue with formatting / naming / ... new-major Change introduced in PR will require a new major release. next-release Issues blocking the next release

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

3 participants