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

Speedup mtype density creation from probability maps using voxcell's ValueToIndexVoxels #79

Merged
merged 6 commits into from
Apr 15, 2024

Conversation

eleftherioszisis
Copy link
Contributor

@eleftherioszisis eleftherioszisis commented Mar 27, 2024

Comparison on 70 processes:

Before : 6276s
After : 60s
Speedup : ~100x

Script used for profiling (Before time was taken from here):

from pathlib import Path
from atlas_densities.app import mtype_densities
from click.testing import CliRunner

OUTPUT_DIR = str(Path(__file__).parent / "out")
DATA_DIR = "/gpfs/bbp.cscs.ch/data/project/proj84/atlas_pipeline_runs/2024-03-11T09:54:30/"
CONFIG_DIR = DATA_DIR + "rules_config_dir/mtypes/"
DENSITY_DIR = DATA_DIR + "inhibitory_neuron_densities_linprog_correctednissl_validated/"

arglist = [
    "--log-output-path",
    OUTPUT_DIR,
    "-v",
    "create-from-probability-map",
    "--hierarchy-path", DATA_DIR + "hierarchy_ccfv2_l23split_barrelsplit.json",
    "--annotation-path", DATA_DIR + "annotation_ccfv2_l23split_barrelsplit_validated.nrrd",
    "--probability-map", CONFIG_DIR + "probability_map_L1.csv",
    "--probability-map", CONFIG_DIR + "probability_map_L23.csv",
    "--probability-map", CONFIG_DIR + "probability_map_L4.csv",
    "--probability-map", CONFIG_DIR + "probability_map_L5.csv",
    "--probability-map", CONFIG_DIR + "probability_map_L6.csv",
    "--probability-map", CONFIG_DIR + "probability_map_TH_INH.csv",
    "--marker", "gad67", DENSITY_DIR + "gad67+_density.nrrd",
    "--marker", "pv", DENSITY_DIR + "pv+_density.nrrd",
    "--marker", "sst", DENSITY_DIR + "sst+_density.nrrd",
    "--marker", "vip", DENSITY_DIR + "vip+_density.nrrd",
    "--synapse-class", "INH",
    "--n-jobs", "70",
    "--output-dir", OUTPUT_DIR,
]
print(" ".join(arglist))

res = CliRunner().invoke(
    mtype_densities.app,
    arglist,
    catch_exceptions=False,
    color=True,
)

@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@fba1e3f). Click here to learn what that means.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #79   +/-   ##
=======================================
  Coverage        ?   97.68%           
=======================================
  Files           ?       22           
  Lines           ?     1509           
  Branches        ?        0           
=======================================
  Hits            ?     1474           
  Misses          ?       35           
  Partials        ?        0           
Flag Coverage Δ
pytest 97.68% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@eleftherioszisis eleftherioszisis requested a review from mgeplf April 3, 2024 14:20
@eleftherioszisis eleftherioszisis requested review from mgeplf and removed request for mgeplf April 12, 2024 06:43
@mgeplf
Copy link
Collaborator

mgeplf commented Apr 15, 2024

I ran the old code path, and the new ones, and all the outputs are exactly the same.
The runtime is very much improved:
Old: python3 test.py 303289.59s user 825.91s system 5872% cpu 1:26:18.56 total
New python3 test.py 2073.11s user 142.77s system 2978% cpu 1:14.40 total

@eleftherioszisis eleftherioszisis merged commit 924b6c6 into main Apr 15, 2024
5 checks passed
@eleftherioszisis eleftherioszisis deleted the optimized-me-densities branch April 15, 2024 11:40
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.

3 participants