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

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 25%. Comparing base (4ebc024) to head (8a2c295).
⚠️ Report is 23 commits behind head on devel.

Additional details and impacted files
@@         Coverage Diff          @@
##           devel   #107   +/-   ##
====================================
  Coverage     25%    25%           
====================================
  Files         25     25           
  Lines       1772   1774    +2     
====================================
+ Hits         435    440    +5     
+ Misses      1337   1334    -3     

☔ 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

ehrenfeu commented Jan 14, 2026

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.

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

@ehrenfeu
Copy link
Member

Merged back devel into this PR in order to fix the linting issues and to prevent merge conflicts.

🚨 @CellKai please make sure to git pull before working on the unit-tests in this PR!! 🚨

@ehrenfeu ehrenfeu added next-release Issues blocking the next release unit testing A unit test should be created changelog Needs to be mentioned in release changelogs and removed blocked Blocked by another issue / PR bug Something isn't working labels Jan 21, 2026
@ehrenfeu
Copy link
Member

Test code at

ff. and will need to be adapted.

@CellKai CellKai requested a review from ehrenfeu January 21, 2026 14:01
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 next-release Issues blocking the next release unit testing A unit test should be created

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

2 participants