Skip to content
This repository has been archived by the owner on Jan 3, 2024. It is now read-only.

ensure cellfinder-core can run on single cpu #198

Merged
merged 5 commits into from
Jul 19, 2023
Merged

Conversation

alessandrofelder
Copy link
Member

@alessandrofelder alessandrofelder commented Jul 13, 2023

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?
It's currently possible for cellfinder-core to spawn a threadpool with 0 processes.
See brainglobe/cellfinder-napari#157

What does this PR do?
Ensures n_ball_procs is always at least 1, and therefore a thread pool with 0 processes is never spawned.

References

Closes #197 and should get us most of the way to brainglobe/cellfinder-napari#157

How has this PR been tested?

Added a test parameter to check this edge case

Is this a breaking change?

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

@alessandrofelder alessandrofelder self-assigned this Jul 13, 2023
avoids n_ball_procs becoming zero if only one cpu available
parametrise integration test to run on both many and one cpu
@alessandrofelder alessandrofelder marked this pull request as ready for review July 13, 2023 17:20
@alessandrofelder
Copy link
Member Author

Still need to test this on a real-life dataset (first thing tomorrow) - will confirm here.

@adamltyson
Copy link
Member

Should we add tests for cellfinder-napari in the same way we do for cellfinder? Or just wait until we merge cellfinder-core & cellfinder-napari?

@alessandrofelder
Copy link
Member Author

Should we add tests for cellfinder-napari in the same way we do for cellfinder? Or just wait until we merge cellfinder-core & cellfinder-napari?

I think it's not much work to add the additional tests to this workflow, and takes pressure off the merging repo timeline, so a good idea? Opened brainglobe/cellfinder#277

@alessandrofelder alessandrofelder mentioned this pull request Jul 14, 2023
7 tasks
@alessandrofelder
Copy link
Member Author

Confirming this ran on real-life large test data on 1 CPU and gave the same result as with 17 CPUs (took ~2:15 h instead of 1:05 h)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cellfinder core should run on just one cpu
2 participants