Skip to content
This repository was archived by the owner on Feb 26, 2025. It is now read-only.

Cache properties accessing libsonata methods#211

Merged
GianlucaFicarelli merged 3 commits into
masterfrom
more_cached_properties
Jun 12, 2023
Merged

Cache properties accessing libsonata methods#211
GianlucaFicarelli merged 3 commits into
masterfrom
more_cached_properties

Conversation

@GianlucaFicarelli

@GianlucaFicarelli GianlucaFicarelli commented May 22, 2023

Copy link
Copy Markdown
Contributor

If nodes and edges _population and _properties are accessed several times, there can be a small performance improvement if they are cached.

For a given circuit, the returned objects should always be the same, and the memory occupation should be minimal compared to the circuit data, so caching them shouldn't be an issue.

Some basic tests:

import bluepysnap
path = "/gpfs/bbp.cscs.ch/project/proj30/tickets/BLPY-289_improve_get/circuit/circuit_config.json"
c = bluepysnap.Circuit(path)
nodes = c.nodes["root__neurons"]

%time nodes._population
%time nodes._properties
%time for i in range(1000): nodes._population
%time for i in range(1000): nodes._properties

Results @master:

CPU times: user 1.45 ms, sys: 0 ns, total: 1.45 ms
Wall time: 1.48 ms
CPU times: user 7 µs, sys: 13 µs, total: 20 µs
Wall time: 23.1 µs
CPU times: user 156 ms, sys: 105 ms, total: 260 ms
Wall time: 266 ms
CPU times: user 1.64 ms, sys: 0 ns, total: 1.64 ms
Wall time: 1.65 ms

Results @more_cached_properties

CPU times: user 841 µs, sys: 632 µs, total: 1.47 ms
Wall time: 1.5 ms
CPU times: user 14 µs, sys: 24 µs, total: 38 µs
Wall time: 42.4 µs
CPU times: user 77 µs, sys: 0 ns, total: 77 µs
Wall time: 81.3 µs
CPU times: user 78 µs, sys: 0 ns, total: 78 µs
Wall time: 80.8 µs

@joni-herttuainen

joni-herttuainen commented May 23, 2023

Copy link
Copy Markdown
Contributor

Since this was discussed earlier with @mgeplf in another PR, I let him to make the call whether we should use cached_property or not.

@GianlucaFicarelli

GianlucaFicarelli commented May 23, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Sure, I can also add more context: in #208 the node population would be retrieved each time get() is called, to access select_all(), attribute_names, and dynamics_attribute_names.

In a script like the one provided in #195, get() is called multiple times, so the times would sum up when using the code in #208. I needed to profile the code to find where the time was spent, because I didn't expect it was in the _population property.

In real use cases, I think that get() isn't called so many times, but it seems safe and harmless to use a cached_property even in this case.

@GianlucaFicarelli GianlucaFicarelli mentioned this pull request Jun 2, 2023
Merged
mgeplf
mgeplf reviewed Jun 5, 2023
View reviewed changes
Comment thread bluepysnap/edges/edge_population.py
@GianlucaFicarelli GianlucaFicarelli force-pushed the more_cached_properties branch from 3094cc0 to 69dfe43 Compare June 9, 2023 12:56
@codecov-commenter

codecov-commenter commented Jun 9, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #211 (69dfe43) into master (2a510ac) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 69dfe43 differs from pull request most recent head 4dabdba. Consider uploading reports for the commit 4dabdba to get more accurate results

@@            Coverage Diff            @@
##            master      #211   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           29        29           
  Lines         2159      2163    +4     
=========================================
+ Hits          2159      2163    +4
Flag Coverage Δ
pytest 100.00% <100.00%> (ø)

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

Impacted Files Coverage Δ
bluepysnap/edges/edge_population.py 100.00% <100.00%> (ø)
bluepysnap/nodes/node_population.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@GianlucaFicarelli GianlucaFicarelli merged commit 0b59c4d into master Jun 12, 2023
@GianlucaFicarelli GianlucaFicarelli deleted the more_cached_properties branch June 12, 2023 09:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@joni-herttuainen joni-herttuainen Awaiting requested review from joni-herttuainen
1 more reviewer
@mgeplf mgeplf mgeplf approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@GianlucaFicarelli @joni-herttuainen @codecov-commenter @mgeplf