[BBPBGLIB-1057] Replace Report.hoc by report.py #145

jorblancoa · 2024-03-20T18:42:38Z

Context

Create a python report.py and replace the old Report.hoc

Review

PR description is complete
Coding style (imports, function length, New functions, classes or files) are good
Unit/Scientific test added
Updated Readme, in-code, developer documentation

matz-e

The less HOC the better 🥳

neurodamus/node.py

neurodamus/report.py

neurodamus/target_manager.py

ferdonline · 2024-04-02T08:42:06Z

Great change! My only slight concern is regarding performance since reports can demanding as they go through all sections and hoc can use optimized 'for' loops, like forsec. I know it's tedious, but could you have maybe a simple time measurement, showing that we don't have a regression in terms of report setting-up time?

jorblancoa · 2024-04-02T10:41:02Z

Great change! My only slight concern is regarding performance since reports can demanding as they go through all sections and hoc can use optimized 'for' loops, like forsec. I know it's tedious, but could you have maybe a simple time measurement, showing that we don't have a regression in terms of report setting-up time?

Actually we have the times of the report setup time. It is true that is slower, but do we care that much? it is usually insignificant compared to the run time.

I checked the blueconfigs sonataconf-quick-v5-plasticity that has 5 reports, and run a simulation for 10ms and changed the target to Mosaic (~220k cells and 110 gids per rank), so this is almost worst case scenario (short simulation with a lot of reports and big number of gids per rank).

The hoc version:

[INFO] +================================================ TIMEIT STATS =================================================+
[INFO] |                       Event Label                        | Avg.Time | Min.Time | Max.Time | Hits R0 / Total   |
[INFO] +---------------------------------------------------------------------------------------------------------------+
[INFO] | Compute LB                                               |     0.02 |     0.01 |     0.03 |       1 / 2.00K   |
[INFO] | Cell creation                                            |    44.23 |    44.23 |    44.23 |       1 / 2.00K   |
[INFO] | Synapse creation                                         |    43.57 |    43.57 |    43.57 |       1 / 2.00K   |
[INFO] | Replay init                                              |     0.05 |     0.03 |     0.06 |       3 / 6.00K   |
[INFO] | Replay inject                                            |     0.16 |     0.14 |     0.17 |       1 / 2.00K   |
[INFO] | Enable Stimulus                                          |     1.16 |     1.12 |     1.68 |       1 / 2.00K   |
[INFO] | Enable Modifications                                     |     0.00 |     0.00 |     0.00 |       1 / 2.00K   |
[INFO] | Enable Reports                                           |    18.81 |    18.59 |    19.03 |       1 / 2.00K   |
[INFO] | Model Finalized                                          |    14.30 |    14.30 |    14.30 |       1 / 2.00K   |
[INFO] |  ╚ stdinit                                               |    14.25 |    14.25 |    14.25 |       1 / 2.00K   |
[INFO] | finished Run                                             |   148.49 |   148.49 |   148.49 |       1 / 2.00K   |
[INFO] |  ╚ psolve                                                |   146.57 |   146.57 |   146.60 |       1 / 2.00K   |
[INFO] +---------------------------------------------------------------------------------------------------------------+

And the python version:

[INFO] +================================================ TIMEIT STATS =================================================+
[INFO] |                       Event Label                        | Avg.Time | Min.Time | Max.Time | Hits R0 / Total   |
[INFO] +---------------------------------------------------------------------------------------------------------------+
[INFO] | Compute LB                                               |     0.02 |     0.01 |     0.03 |       1 / 2.00K   |
[INFO] | Cell creation                                            |    44.52 |    44.52 |    44.52 |       1 / 2.00K   |
[INFO] | Synapse creation                                         |    40.99 |    40.99 |    40.99 |       1 / 2.00K   |
[INFO] | Replay init                                              |     0.04 |     0.03 |     0.05 |       3 / 6.00K   |
[INFO] | Replay inject                                            |     0.16 |     0.15 |     0.18 |       1 / 2.00K   |
[INFO] | Enable Stimulus                                          |     1.16 |     1.12 |     1.58 |       1 / 2.00K   |
[INFO] | Enable Modifications                                     |     0.00 |     0.00 |     0.00 |       1 / 2.00K   |
[INFO] | Enable Reports                                           |    31.88 |    31.64 |    32.09 |       1 / 2.00K   |
[INFO] | Model Finalized                                          |    13.75 |    13.75 |    13.75 |       1 / 2.00K   |
[INFO] |  ╚ stdinit                                               |    13.71 |    13.71 |    13.71 |       1 / 2.00K   |
[INFO] | finished Run                                             |   144.41 |   144.41 |   144.41 |       1 / 2.00K   |
[INFO] |  ╚ psolve                                                |   143.83 |   143.83 |   143.85 |       1 / 2.00K   |
[INFO] +---------------------------------------------------------------------------------------------------------------+

What do you think?

ferdonline · 2024-04-03T09:09:53Z

Thanks for the measurements. I think that's acceptable given the benefit. 👍

Address comments

- Move exception handling outside report_setup and avoid returning a boolean BLUECONFIGS_BRANCH=jblanco/summation_intrinsic_currents_precision

BLUECONFIGS_BRANCH=jblanco/summation_intrinsic_currents_precision

bbpbuildbot · 2024-04-16T13:48:43Z

Logfiles from GitLab pipeline #205492 (:white_check_mark:) have been uploaded here!

Status and direct links:

## Context Create a python report.py and replace the old Report.hoc ## Review * [x] PR description is complete * [x] Coding style (imports, function length, New functions, classes or files) are good * [x] Unit/Scientific test added * [ ] Updated Readme, in-code, developer documentation

This comment has been minimized.

Sign in to view

matz-e reviewed Mar 22, 2024

View reviewed changes

neurodamus/node.py Outdated Show resolved Hide resolved

neurodamus/node.py Outdated Show resolved Hide resolved

neurodamus/node.py Outdated Show resolved Hide resolved

neurodamus/node.py Show resolved Hide resolved

neurodamus/report.py Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

jorblancoa force-pushed the jblanco/report_refactor branch from ea1e422 to 2296f55 Compare March 26, 2024 10:30

jorblancoa marked this pull request as ready for review March 26, 2024 10:30

This comment has been minimized.

Sign in to view

jorblancoa force-pushed the jblanco/report_refactor branch from 2296f55 to f11bdc4 Compare March 26, 2024 16:00

This comment has been minimized.

Sign in to view

jorblancoa requested review from matz-e, WeinaJi and jamesgking March 26, 2024 17:03

matz-e previously approved these changes Mar 28, 2024

View reviewed changes

neurodamus/target_manager.py Show resolved Hide resolved

jorblancoa dismissed matz-e’s stale review via f0a5936 April 9, 2024 14:27

jorblancoa force-pushed the jblanco/report_refactor branch from f0a5936 to 79b6682 Compare April 9, 2024 14:28

WeinaJi previously approved these changes Apr 9, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

jorblancoa dismissed WeinaJi’s stale review via a5b4db1 April 9, 2024 14:54

WeinaJi previously approved these changes Apr 9, 2024

View reviewed changes

jorblancoa force-pushed the jblanco/report_refactor branch from a5b4db1 to 26b62e6 Compare April 9, 2024 15:12

This comment has been minimized.

Sign in to view

jorblancoa dismissed WeinaJi’s stale review via a9f23d5 April 9, 2024 15:40

This comment has been minimized.

Sign in to view

jorblancoa force-pushed the jblanco/report_refactor branch from a9f23d5 to 45bbbbe Compare April 9, 2024 20:57

This comment has been minimized.

Sign in to view

WeinaJi approved these changes Apr 12, 2024

View reviewed changes

jorblancoa added 11 commits April 16, 2024 15:02

Replace Report.hoc by report.py

97e9e3a

Remove TType.hoc and TPointList.hoc

3430f76

fixup! Remove TType.hoc and TPointList.hoc

383d691

fixup! Remove TType.hoc and TPointList.hoc

f844584

Create report only when the rank has gids

9c63980

Fix summation issue when order or process currents affected results

7d44dcf

Address comments

Add report unit tests

cba85c3

Address feedback

f61ff4f

- Move exception handling outside report_setup and avoid returning a boolean BLUECONFIGS_BRANCH=jblanco/summation_intrinsic_currents_precision

Raise Exception when there is an error in the report setup

3714fc5

fixup! Raise Exception when there is an error in the report setup

a582c96

fixup! Raise Exception when there is an error in the report setup

7bbf3cc

BLUECONFIGS_BRANCH=jblanco/summation_intrinsic_currents_precision

jorblancoa force-pushed the jblanco/report_refactor branch from 45bbbbe to 7bbf3cc Compare April 16, 2024 13:02

jorblancoa merged commit 3c1f9cc into main Apr 17, 2024
6 checks passed

jorblancoa deleted the jblanco/report_refactor branch April 17, 2024 09:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BBPBGLIB-1057] Replace Report.hoc by report.py #145

[BBPBGLIB-1057] Replace Report.hoc by report.py #145

jorblancoa commented Mar 20, 2024 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

matz-e left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ferdonline commented Apr 2, 2024

jorblancoa commented Apr 2, 2024

ferdonline commented Apr 3, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bbpbuildbot commented Apr 16, 2024

[BBPBGLIB-1057] Replace Report.hoc by report.py #145

[BBPBGLIB-1057] Replace Report.hoc by report.py #145

Conversation

jorblancoa commented Mar 20, 2024 • edited Loading

Context

Review

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

matz-e left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ferdonline commented Apr 2, 2024

jorblancoa commented Apr 2, 2024

ferdonline commented Apr 3, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bbpbuildbot commented Apr 16, 2024

jorblancoa commented Mar 20, 2024 •

edited

Loading