-
Notifications
You must be signed in to change notification settings - Fork 38
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
Impedance ("Z score") feature is added #165
Conversation
efel/pyfeatures/pyfeatures.py
Outdated
freq = numpy.fft.fftfreq(len(volt), d=0.0001) | ||
Z = ((fft_volt) / (fft_cur)) | ||
signalPhase = numpy.angle(fft_volt) | ||
norm_Z = abs(Z[1:150]) / max(abs(Z[1:150])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have to hardcode 150 here ? It should at least be a general eFEL setting
I was trying to undo the _versioneer change. Let me reopen it. |
I don't have permission to push, @mariarv did you enable the "allow contributors to make changes" option? |
The issue was automatically getting closed when I wanted to push after a rebase while my default git push behaviour was |
Impedance feature PR BlueBrain#165 merged with master
The only conflict with master is the presence of "impedance" feature in the |
Sure, but that file should be changed in a way that it doesn't cause a merge conflict with getting that into master |
It's tricky: |
I can merge master here. I think that would be the best. |
But I think there is something wrong with the exact line or so. I've never seen a merge conflict when adding features. |
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 4117769, git still kept the conflict statement |
That looks better :-) |
efel/pyfeatures/pyfeatures.py
Outdated
normalized_voltage = voltage_trace - holding_voltage | ||
current_trace = current() | ||
if current_trace is not None: | ||
current_base = numpy.median(current_trace[0:10]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we still need a current_base feature for this, the 0:10 should not be hardcoded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and also the "median" option for both voltage_base and current_base features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we can use the current base feature with median but how can we set the range [0:10] there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mariarv should this be 0:10? Wouldn't it be better to use the same range as voltage_base?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started using current_base in latest commit
efel/pyfeatures/pyfeatures.py
Outdated
Z = fft_volt / fft_cur | ||
norm_Z = abs(Z) / max(abs(Z)) | ||
# physiological range of frequencies range [0,100] | ||
smooth_Z = gaussian_filter1d(norm_Z[0:100], 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this range should still become an efel setting, so that we don't hardcoded '100'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in latest commit. New efel setting is impedance_max_freq
and its default value is 50
, to be consistent with this paper: https://pubmed.ncbi.nlm.nih.gov/1992481/
efel/pyfeatures/pyfeatures.py
Outdated
normalized_voltage = voltage_trace - holding_voltage | ||
current_trace = current() | ||
if current_trace is not None: | ||
current_base = numpy.median(current_trace[0:10]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this should be called holding_current to be consistent with holding_voltage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in latest commit
Codecov ReportAttention:
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
☔ View full report in Codecov by Sentry. |
…-master Conflicts: efel/pyfeatures/pyfeatures.py tests/test_pyfeatures.py
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good thanks everyone
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.