Skip to content

Conversation

@CellKai
Copy link
Contributor

@CellKai CellKai commented Aug 18, 2025

Needed because the current function is ignoring the user parameter and always splits by time point.

@codecov
Copy link

codecov bot commented Aug 18, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 25%. Comparing base (26f64c9) to head (5a97a7b).

Files with missing lines Patch % Lines
src/imcflibs/imagej/bdv.py 67% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##           master   #107   +/-   ##
=====================================
  Coverage      25%    25%           
=====================================
  Files          25     25           
  Lines        1688   1691    +3     
=====================================
+ Hits          421    423    +2     
- Misses       1267   1268    +1     

☔ 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.

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.

Would be nice to have this behavior explained in the function's docstring.

@ehrenfeu ehrenfeu added this to the 2.0.0 milestone Jan 14, 2026
@ehrenfeu ehrenfeu moved this to In review in imcflibs Jan 14, 2026
@ehrenfeu ehrenfeu added the bug Something isn't working label 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.

Actually, by its nature the parameter timepoints_per_partition should rather be boolean than int as it's only having two values with the semantics "split" or "don't split"...

Let's do this now as we're anyway aiming for a major release.

@ehrenfeu ehrenfeu added the blocked Blocked by another issue / PR label Jan 14, 2026
@ehrenfeu ehrenfeu moved this from In review to In progress in imcflibs Jan 14, 2026
@ehrenfeu
Copy link
Member

Got it wrong, it is indeed an int as it can also take numbers larger than 1, resulting in multiple timepoints being aggregated into a single HDF5 file.

➡️ Explain this in the docstring instead, but leave the code as it is.

@ehrenfeu ehrenfeu changed the base branch from master to devel January 14, 2026 14:04
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.

🚨 The default behavior changed, so this has to be mentioned in the changelog (#126).

@ehrenfeu
Copy link
Member

Pytest 🕵🏼 is failing, do we need to update the testing scripts to pull in the latest BDV / Fiji?

I guess this will also be the case for other PR's...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocked Blocked by another issue / PR bug Something isn't working

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

2 participants