Impedance ("Z score") feature is added

[mariarv]

The "impedance" feature takes the maximum of impedance of the neuron. Impedance is calculated by dividing the power spectrum of the voltage trace by the power spectrum of ZAP current. An example of impedance profile and its maximum (star symbol on the plot) for corresponding current and voltage traces is attached.

[anilbey]

I was trying to undo the _versioneer change. Let me reopen it.

[anilbey]

I don't have permission to push, @mariarv did you enable the "allow contributors to make changes" option?

[anilbey]

The issue was automatically getting closed when I wanted to push after a rebase while my default git push behaviour was matching. Now I set it to simple (I can push now) but the PR needs to be reopened.

[codecov-io]

Codecov Report

Merging #165 into master will decrease coverage by 0.56%. The diff coverage is 24.00%.

@@            Coverage Diff             @@
##           master     #165      +/-   ##
==========================================
- Coverage   66.64%   66.08%   -0.57%     
==========================================
  Files          12       12              
  Lines        1970     1993      +23     
==========================================
+ Hits         1313     1317       +4     
- Misses        657      676      +19     

Impacted Files	Coverage Δ
efel/pyfeatures/pyfeatures.py	0.00% <0.00%> (ø)
efel/tests/test_allfeatures.py	96.03% <100.00%> (ø)
efel/tests/test_cppcore.py	100.00% <100.00%> (ø)
efel/tests/test_pyfeatures.py	100.00% <100.00%> (ø)

---

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 af8a005...4117769. Read the comment docs.

[anilbey]

The C++ implementation of voltage base is using the mean but the impedance feature is assuming median. What shall we do?

https://github.com/BlueBrain/eFEL/blob/760ec49235f480e30481ca1138a92bb4ac4a1f88/efel/cppcore/LibV5.cpp#L2261-L2280

[wvangeit]

About mean / median, just discussed this with @mariarv , I think it's best to add a new setting to efel like these https://github.com/BlueBrain/eFEL/blob/master/efel/api.py#L88 to let the user choose between median or mean for voltage_base.

[wvangeit]

(btw, the merge button is warning about a conflict with the master branch atm)

[anilbey]

The conflict is just the ordering of featurenames.json. I sorted it on master branch.

[anilbey]

Is it going to be like setDoubleSetting("voltage_base", "median")?

[anilbey]

Travis is not running. This link seems to have a solution for that but it requires owner rights to change the project setting. https://github.com/travis-ci/travis-ci/issues/10204

[wvangeit]

Mmm, I checked, but these setting were set like in the mentioned issue

[wvangeit]

Mmm, travis logs say 'job rejected', let me check

[wvangeit]

It says " GitHub payload is missing a merge commit (mergeable_state: "unknown", merged: false) "

[wvangeit]

Did you already fix the merge problem with the master? Maybe that might help

[wvangeit]

That (fix merge with master) will probably solve it: "Travis builds a “merge preview” commit provided by Github for PRs, so if it’s impossible to merge the PR, it’s impossible to build it." https://travis-ci.community/t/travis-ci-stopped-tracking-building-a-pr/4159/2

[anilbey]

The only conflict with master is the presence of "impedance" feature in the featurenames.json.

[wvangeit]

Sure, but that file should be changed in a way that it doesn't cause a merge conflict with getting that into master

[anilbey]

It's tricky: If I merge it by keeping "impedance", master branch will fail since "impedance" feature does not exist. If I merge it by removing "impedance", this branch will fail.

[anilbey]

I can merge master here. I think that would be the best.

[wvangeit]

But I think there is something wrong with the exact line or so. I've never seen a merge conflict when adding features.

[anilbey]

Previously the sorting order of lines was different and there was also the removal of duplicates. That had created the original merge conflict and after resolving it in 41177691857f9de2a2df1a3eeb015cb64305fc3c, git still kept the conflict statement

[wvangeit]

That looks better :-)

[codecov-commenter]

Codecov Report

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

Comparison is base (4dfe0e4) 53.45% compared to head (cdaec47) 53.59%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #165      +/-   ##
==========================================
+ Coverage   53.45%   53.59%   +0.14%     
==========================================
  Files          30       30              
  Lines        8394     8426      +32     
  Branches     3677     3677              
==========================================
+ Hits         4487     4516      +29     
- Misses       1261     1264       +3     
  Partials     2646     2646              

Files	Coverage Δ
efel/api.py	98.77% <100.00%> (+<0.01%)	:arrow_up:
tests/test_allfeatures.py	96.03% <ø> (ø)
tests/test_pyfeatures.py	100.00% <100.00%> (ø)
efel/pyfeatures/pyfeatures.py	90.56% <88.88%> (-0.35%)	:arrow_down:

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[AurelienJaquier]

Hi there! People from BBP are starting to do single cell optimisation and they will need this feature, so I will make some changes to this PR soon. I just have one question: @mariarv is there a reason you take into account the 0 Hz frequency here? I believe impedance at this frequency it is just resistance, and the few references I read about neuron impedance were not using it to compute Z. Also, when taking it into account, the smoothed Z at 0 Hz becomes very close to the maximum Z, thus we might have a problem for other traces if Z(0 Hz) becomes the max. See below plots using your data for Z with / without 0Hz datapoint.

(1st plot: without 0Hz) (2nd plot: with 0Hz)

[mariarv]

Hi @AurelienJaquier. Sure, you can get rid of 0 Hz, ideally it should not matter, as the peak of impedance shall not be at 0 Hz. Cheers, Maria

[AurelienJaquier]

Thanks @mariarv for the clarifications.

I think this PR is good to be merged. Maybe @anilbey could give it a final check?