Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Benchmark for cellfinder workflow with CLI arguments #24

Merged
merged 24 commits into from
Oct 16, 2023

Conversation

sfmig
Copy link
Contributor

@sfmig sfmig commented Oct 9, 2023

Adds asv benchmarks to time the cellfinder workflows defined in PR #23.

We benchmark the full workflow and its individual steps (reading, detecting cells and saving results to file).

This PR also includes some refactoring changes to the cellfinder workflows, to allow us to split the setup steps into data downloading and other steps. This way we can download the data only once for all the repeats of a benchmark.

Would need rebasing after PR #23 is squash-and-merged

To run the benchmarks, run asv run from the directory where asv.conf.json

@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (f16c6b9) 92.56% compared to head (d83f97b) 92.80%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #24      +/-   ##
==========================================
+ Coverage   92.56%   92.80%   +0.23%     
==========================================
  Files           2        2              
  Lines         121      125       +4     
==========================================
+ Hits          112      116       +4     
  Misses          9        9              
Files Coverage Δ
brainglobe_workflows/cellfinder/cellfinder_main.py 94.16% <89.06%> (+0.20%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sfmig sfmig marked this pull request as ready for review October 12, 2023 11:29
@sfmig sfmig force-pushed the smg/cellfinder-cli-benchmark branch from 00970cc to 76352ac Compare October 13, 2023 13:38
Copy link
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.

Tiny comment that I am not sure about.
Otherwise LGTM 🚀

asv.conf.json Outdated

// List of branches to benchmark. If not provided, defaults to "master"
// (for git) or "default" (for mercurial).
"branches": ["smg/cellfinder-workflow"], // for git
Copy link
Member

Choose a reason for hiding this comment

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

Is this right? (It's a different name to the branch of the PR and I had to change it to run it locally?)

Copy link
Contributor Author

@sfmig sfmig Oct 13, 2023

Choose a reason for hiding this comment

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

yes sorry, I think that came about from the rebasing and merging of conflicts 🤔
it should have been smg/cellfinder-workflow-cli-w-tests but now that is merged main should work

actually it won't work with main just yet, since I refactored the workflows in this PR 😓

@alessandrofelder
Copy link
Member

(Also, remember to rebase I guess @sfmig ?)

@sfmig sfmig mentioned this pull request Oct 16, 2023
@sfmig sfmig merged commit 759f245 into main Oct 16, 2023
10 of 11 checks passed
@alessandrofelder alessandrofelder deleted the smg/cellfinder-cli-benchmark branch October 23, 2023 10:05
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.

None yet

2 participants