Skip to content

Conversation

@matham
Copy link
Contributor

@matham matham commented Jun 17, 2025

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

Cellfinder recently got / is in the process of getting new args. This adds support for those, including updating the docs to match those from cellfinder.

Also, cellfinder was called using positional args instead of keyword args, making adding new args to cellfinder harder. This switches to only use keyword args.

What does this PR do?

  1. Converts positional args to keyword args.
  2. Adds support for the new args.
  3. Updates the docs for the cellfinder args.
  4. Added both detection_batch_size and classification_batch_size, instead of the singular batch_size. And marked the latter as deprecated. Python 3.13+ has an actual deprecated arg we can use, but we can't use it yet. If batch_size is provided, it still sets classification_batch_size like before.

References

It's WIP until these PRs with potential new parameters are resolved: brainglobe/cellfinder#542, brainglobe/cellfinder#545, and brainglobe/cellfinder#546.

Also, it requires brainglobe/brainglobe-utils#128.

How has this PR been tested?

I tested it with data locally.

Is this a breaking change?

It only breaks things in the sense that it will require the latest version of cellfinder and brainglobe-utils. Because older versions won't support these function calls. We probably should set the min versions in the dependencies.

Does this PR require an update to the documentation?

No.

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

@matham matham marked this pull request as draft June 17, 2025 18:53
@alessandrofelder
Copy link
Member

Thanks @matham - I have merged brainglobe/brainglobe-utils#128 and released a new brainglobe-utils version in the hope to unblock this! Let us know if you are able to continue here - and apologies for taking so long to get to this!

@matham
Copy link
Contributor Author

matham commented Jan 21, 2026

Thanks, I'll revive the PR. Or rather I'll probably make a new PR because I used this a bit of a staging branch including args for stuff I haven't made a PR for.

I did wonder about the approach to keep adding args to brainmapper. It already made me a bit uncomfortable to keep stuffing in new global arguments to brainmapper, and I've recently used click, which supports sub-commands with shared global parameters as well as parameters that are specific to sub-commands.

I know we probably don't want to re-write brainmapper from scratch especially since there also seems to be a second command line tool in this repo, with all the json config files etc, but perhaps we should consider a different way to run these commands? Maybe even click which is quite nice, since from my use it seemed almost perfectly suited for this (it supports providing command line options using env variables and extending to config files is easy).

But I'm not volunteering to do that...so I can just polish up this PR if we're okay with adding more variables to brainmapper.

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.

2 participants