Skip to content

Fix #605: make background_array optional when skip_classification=True#607

Open
KpradeepKumar25 wants to merge 2 commits into
brainglobe:mainfrom
KpradeepKumar25:fix-605-optional-background-array
Open

Fix #605: make background_array optional when skip_classification=True#607
KpradeepKumar25 wants to merge 2 commits into
brainglobe:mainfrom
KpradeepKumar25:fix-605-optional-background-array

Conversation

@KpradeepKumar25
Copy link
Copy Markdown

@KpradeepKumar25 KpradeepKumar25 commented Mar 21, 2026

Background_array was always required even when skip_classification=True. This fix makes it default to None and adds
ValueError with a clear message when classification is attempted without a background array.

Fixes #605

Before submitting a pull request (PR), please read the contributing guide.

Please fill out as much of this template as you can, but if you have any problems or questions, just leave a comment and we will help out :)

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

background_array was always a required argument even when skip_classification=True. This blocked researchers who only have
a single channel (signal only) from using cellfinder for detection without classification.

What does this PR do?

  • Makes background_array default to None instead of being required
  • Adds a clear ValueError message if someone tries to run
    classification without providing background_array
  • Updates the docstring to reflect that background_array is optional

References

Fixes #605
Related to #352

How has this PR been tested?

Manually verified that calling main() with skip_classification=True and no background_array no longer raises a TypeError.
Also verified that calling main() with skip_classification=False and no background_array now raises a clear ValueError.

Is this a breaking change?

No. Users who already pass background_array will see no change. Users who pass skip_classification=True without background_array will now get the expected behaviour instead of a TypeError.

Does this PR require an update to the documentation?

The docstring for background_array in main() has been updated to reflect it is now optional.

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality (unit & integration)
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

background_array was always required even when skip_classification=True.
This fix makes it default to None and adds a ValueError with a clear 
message when classification is attempted without a background array.

Fixes brainglobe#605
Copy link
Copy Markdown
Member

@alessandrofelder alessandrofelder left a comment

Choose a reason for hiding this comment

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

Thanks for contributing @KpradeepKumar25

I think it's unfortunately too confusing/not backwards-compatible to switch the order of the positional arguments as this PR suggests. Many workflows that depend on this will need to switch the order of the arguments with which they call the main function if we merged this.

I suggest instead that we keep the order of the arguments, but allow None to be explicitly passed for the background array, rather than being the default.

Also, this PR should add a test that checks that the expected error is raised when background_array=None is passed.

[Edit] I would be grateful if you could also resolve the merge conflicts.

@KpradeepKumar25
Copy link
Copy Markdown
Author

Thank you for the review @alessandrofelder I appreciate the clear feedback. I understand the concern about breaking existing workflows by changing the argument order — I will revert it back to the original position and instead allow None to be passed explicitly for background_array.
I will also add a pytest test to verify the ValueError is raised correctly, and I will resolve the merge conflicts.
I want to be transparent — I am currently in my final semester exams, so I am taking a bit of time to study the codebase more carefully before updating the PR. I want to make sure I understand the full picture properly rather than rushing a fix. I will update the PR as soon as my exams are done. Thank you for your patience!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: background channel is required even when skip_classification=True (single-channel flow breaks)

2 participants