LFP Calculator by bdelmarm · Pull Request #492 · BlueBrain/CoreNeuron

bdelmarm · 2021-02-05T14:30:32Z

New LFP calculator class.

Allows for LineSource and PointSource approximations.

A unit test, testing the component, is available in /tests/unit/lfp.

Documentation below:
LFP.pdf

bbpbuildbot · 2021-02-05T14:30:34Z

Can one of the admins verify this patch?

bbpbuildbot · 2021-02-05T14:30:34Z

Can one of the admins verify this patch?

bbpbuildbot · 2021-02-05T14:30:34Z

Can one of the admins verify this patch?

…ibrary is built

bbpbuildbot · 2021-02-09T16:25:04Z

Build finished.

codecov-io · 2021-02-09T21:59:27Z

Codecov Report

Merging #492 (06c1519) into master (5a93d9b) will increase coverage by 0.90%.
The diff coverage is 93.12%.

@@            Coverage Diff             @@
##           master     #492      +/-   ##
==========================================
+ Coverage   55.96%   56.87%   +0.90%     
==========================================
  Files          95       97       +2     
  Lines        7835     7960     +125     
==========================================
+ Hits         4385     4527     +142     
+ Misses       3450     3433      -17

Impacted Files	Coverage Δ
coreneuron/io/lfp.hpp	`90.66% <90.66%> (ø)`
tests/unit/lfp/lfp.cpp	`96.42% <96.42%> (ø)`
coreneuron/utils/progressbar/progressbar.c	`82.14% <0.00%> (-5.96%)`	⬇️
coreneuron/io/mech_report.cpp	`100.00% <0.00%> (ø)`
coreneuron/io/nrn_setup.cpp	`85.71% <0.00%> (+0.39%)`	⬆️
coreneuron/network/netcvode.cpp	`76.23% <0.00%> (+0.66%)`	⬆️
coreneuron/mpi/nrnmpi.cpp	`54.16% <0.00%> (+2.77%)`	⬆️
coreneuron/mpi/mpispike.cpp	`41.33% <0.00%> (+4.00%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a93d9b...06c1519. Read the comment docs.

castigli

@bdelmarm , overall it looks good, I have some questions about some of the templates though.

cattabiani

Good job! Maybe we can generalize a little the code to take care of also non-3D points (just in case one day someone comes around and needs points in 2D)

- Created new pi global parameter which is read by NEURON globals.dat or directly NEURON in online mode - Use this pi in lfp calculation and test - Removed a deprecated boost test header file

Pass PI value from NEURON to CoreNEURON

castigli

It looks good do me, I am not approving just because there are a couple of points that others (@tristan0x @cattabiani ) should confirm that they are satisfied with.

tristan0x

All symbols inside in coreneuron::lfputils are called either in lfp.cpp or in LFPCalculator::getFactor which is a private member function only called in the compilation unit lfp.cpp. So the user is not concerned by it and there is not benefits in term of performance to have them in the header. This is why I believe they should all be moved to the compilation unit.

I also made a comment regarding performance improvement in LFPCalculator::lfp member method.

cattabiani

LGTM! Just some ordering and the value of pi

cattabiani · 2021-02-24T14:10:43Z

 #include <stdint.h>

 #include <boost/test/unit_test.hpp>
-#include <boost/test/test_case_template.hpp>
 #include <boost/mpl/list.hpp>


maybe lexicographic sort?

Co-authored-by: Tristan Carel <tristan.carel@gmail.com>

tristan0x · 2021-02-24T21:13:36Z

All symbols inside in coreneuron::lfputils are called either in lfp.cpp or in LFPCalculator::getFactor which is a private member function only called in the compilation unit lfp.cpp. So the user is not concerned by it and there is not benefits in term of performance to have them in the header. This is why I believe they should all be moved to the compilation unit.

Following discussion with @bdelmarm , these symbols need to be exposed to be unit-tested.

tristan0x · 2021-02-24T21:14:59Z

I also made a comment regarding performance improvement in LFPCalculator::lfp member method.

adressed in f80818a

cattabiani

I am lost about pi. It is still hard-coded. I thought we were going to get it from neuron...

cattabiani

LGTM

iomaganaris

LGTM! One last small thing

Co-authored-by: Ioannis Magkanaris <iomagkanaris@gmail.com>

* New LFP calculator class - Allows for LineSource and PointSource approximations * Read value of `pi` from NEURON in online or offline mode Co-authored-by: Del Marmol Baudouin <bdelmarm@bbpv1.epfl.ch> Co-authored-by: Pramod S Kumbhar <pramod.s.kumbhar@gmail.com> Co-authored-by: Ioannis Magkanaris <iomagkanaris@gmail.com> Co-authored-by: Tristan Carel <tristan.carel@epfl.ch> Co-authored-by: Tristan Carel <tristan.carel@gmail.com> CoreNEURON Repo SHA: BlueBrain/CoreNeuron@b234502

Del Marmol Baudouin added 4 commits February 5, 2021 12:45

LFP classes + unit test.

6b8e1ea

Cosmetic.

d7f8b72

Cosmetic.

c62ec29

Polishing test. Make it compile.

de64826

Del Marmol Baudouin added 2 commits February 5, 2021 15:46

Unit test CMakeLists.txt.

8f3bd48

Rework LFP using MPI wrappers.

64ae686

bdelmarm force-pushed the sandbox/bdelmarm/lfp_rebase branch from 389a932 to 64ae686 Compare February 8, 2021 13:43

Del Marmol Baudouin and others added 7 commits February 8, 2021 14:57

Clang format.

83ce660

Only use MPI reduction if MPI is available..

9b2204e

Fix previous commit.

5056df6

Clang format.

59a98b8

Apply clang-format

8cd40fa

Apply cmake-format

58fe6f1

Fix the dependency issue : lfp should be compiled after corenrnmech l…

06c1519

…ibrary is built

bdelmarm requested review from castigli, cattabiani, pramodk and tristan0x February 9, 2021 16:53

bdelmarm requested a review from jorblancoa February 10, 2021 01:24

castigli suggested changes Feb 10, 2021

View reviewed changes

Comment thread coreneuron/io/lfp.hpp Outdated

Comment thread coreneuron/io/lfp.hpp Outdated

Comment thread coreneuron/io/lfp.hpp Outdated

Comment thread coreneuron/io/lfp.hpp Outdated

cattabiani suggested changes Feb 10, 2021

View reviewed changes

iomaganaris suggested changes Feb 10, 2021

View reviewed changes

Comment thread coreneuron/io/lfp.hpp Outdated

iomaganaris and others added 3 commits February 10, 2021 17:40

Fixes related to PI value read by NEURON

60bfc66

- Created new pi global parameter which is read by NEURON globals.dat or directly NEURON in online mode - Use this pi in lfp calculation and test - Removed a deprecated boost test header file

Merge pull request #494 from BlueBrain/magkanar/lfp_rebase_pi_fix

8bcc10a

Pass PI value from NEURON to CoreNEURON

Refactoring following review.

91cf705

pramodk requested review from castigli and iomaganaris February 23, 2021 17:45

Remove T type definition.

72d7abe

castigli reviewed Feb 24, 2021

View reviewed changes

tristan0x suggested changes Feb 24, 2021

View reviewed changes

Comment thread coreneuron/io/lfp.hpp Outdated

Comment thread coreneuron/io/lfp.hpp Outdated

Comment thread coreneuron/io/lfp.cpp Outdated

cattabiani reviewed Feb 24, 2021

View reviewed changes

Del Marmol Baudouin added 2 commits February 24, 2021 15:36

Code changes following review.

f80818a

Sort includes.

299f50c

tristan0x suggested changes Feb 24, 2021

View reviewed changes

Comment thread coreneuron/io/lfp.hpp Outdated

tristan0x and others added 5 commits February 24, 2021 15:44

remove semi-colon after closing namespace

0db2067

Update coreneuron/io/lfp.hpp

d747e66

Co-authored-by: Tristan Carel <tristan.carel@gmail.com>

Fix formatting of lfp header and compilation unit

a2e4e82

fixup! Fix formatting of lfp header and compilation unit

e90f822

Fix passing value of pi

19d374e

tristan0x approved these changes Feb 24, 2021

View reviewed changes

cattabiani suggested changes Feb 25, 2021

View reviewed changes

Comment thread tests/unit/lfp/lfp.cpp

castigli approved these changes Feb 25, 2021

View reviewed changes

cattabiani approved these changes Feb 25, 2021

View reviewed changes

iomaganaris added 3 commits February 25, 2021 10:53

Merge branch 'master' into sandbox/bdelmarm/lfp_rebase

026ce08

Add pi in pragma update

1983ca8

Fix in pragma declare

18ca214

pramodk approved these changes Feb 25, 2021

View reviewed changes

iomaganaris approved these changes Feb 25, 2021

View reviewed changes

Comment thread coreneuron/io/lfp.hpp Outdated

Update coreneuron/io/lfp.hpp

cfc7000

Co-authored-by: Ioannis Magkanaris <iomagkanaris@gmail.com>

iomaganaris merged commit b234502 into master Feb 25, 2021

iomaganaris deleted the sandbox/bdelmarm/lfp_rebase branch February 25, 2021 12:57

pramodk mentioned this pull request Jun 7, 2021

Incremental simulation in direct memory mode #416

Closed

Conversation

bdelmarm commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bbpbuildbot commented Feb 5, 2021

Uh oh!

bbpbuildbot commented Feb 5, 2021

Uh oh!

bbpbuildbot commented Feb 5, 2021

Uh oh!

bbpbuildbot commented Feb 9, 2021

Uh oh!

codecov-io commented Feb 9, 2021

Codecov Report

Uh oh!

castigli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cattabiani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

castigli left a comment

Choose a reason for hiding this comment

Uh oh!

tristan0x left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cattabiani left a comment

Choose a reason for hiding this comment

Uh oh!

cattabiani Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

bdelmarm Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tristan0x commented Feb 24, 2021

Uh oh!

tristan0x commented Feb 24, 2021

Uh oh!

cattabiani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cattabiani left a comment

Choose a reason for hiding this comment

Uh oh!

iomaganaris left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

bdelmarm commented Feb 5, 2021 •

edited

Loading