Fix AP_begin_indices-dependent features #minor #353
Conversation
…P-end-indices Conflicts: efel/cppcore/LibV2.cpp efel/cppcore/LibV5.cpp
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #353 +/- ##
==========================================
+ Coverage 65.86% 65.95% +0.09%
==========================================
Files 33 33
Lines 6380 6445 +65
Branches 2262 2284 +22
==========================================
+ Hits 4202 4251 +49
- Misses 262 265 +3
- Partials 1916 1929 +13 ☔ View full report in Codecov by Sentry. |
efel/cppcore/LibV5.cpp
Outdated
| @@ -26,6 +26,7 @@ | |||
| #include <deque> | |||
| #include <functional> | |||
| #include <iterator> | |||
| #include <iostream> | |||
There was a problem hiding this comment.
Was iostream required while debugging?
There was a problem hiding this comment.
oops, I made a few cout while debugging and forgot to remove it. I'll remove it just wait a few secs
efel/cppcore/LibV2.cpp
Outdated
| vector<int> newpeakindices; | ||
| for (size_t i=0; i < peakindices.size(); i++) { | ||
| if (t[peakindices[i]] > stimstart) { | ||
| newpeakindices.push_back(peakindices[i]); | ||
| } | ||
| } |
There was a problem hiding this comment.
This pattern occurs in multiple places. It can be reused now and also in the future. Can you put it into a function?
There was a problem hiding this comment.
Implemented in latest commit. Tell me if it looks good to you
efel/cppcore/Utils.cpp
Outdated
| int peaks_after_stim_start(const double stimstart, | ||
| const vector<int>& peakindices, | ||
| const vector<double>& t, | ||
| vector<int>& newpeakindices){ | ||
| for (size_t i=0; i < peakindices.size(); i++) { | ||
| if (t[peakindices[i]] > stimstart) { | ||
| newpeakindices.push_back(peakindices[i]); | ||
| } | ||
| } | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Thanks, since C++11 this can be shortened as follows. The compiler applies return value optimisation to avoid copy/move operations.
vector<int> peaks_after_stim_start(const double stimstart,
const vector<int>& peakindices,
const vector<double>& t) {
vector<int> newpeakindices;
for (size_t i = 0; i < peakindices.size(); i++) {
if (t[peakindices[i]] > stimstart) {
newpeakindices.push_back(peakindices[i]);
}
}
return newpeakindices;
}and the usage can be simplified like this
vector<int> newpeakindices = peaks_after_stim_start(stimstart, peakindices, t);There was a problem hiding this comment.
done in latest commit
efel/cppcore/Utils.cpp
Outdated
| int peaks_after_stim_start(const double stimstartindex, | ||
| const vector<int>& peakindices, | ||
| vector<int>& newpeakindices){ | ||
| for (size_t i=0; i < peakindices.size(); i++) { | ||
| if (peakindices[i] > stimstartindex) { | ||
| newpeakindices.push_back(peakindices[i]); | ||
| } | ||
| } | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Instead of the above function can't this one be used if t[peakindices[i]] is sent as an argument (instead of both t and peakindices)?
There was a problem hiding this comment.
I'm sorry, I'm not sure I understand. We don't have i yet when we call the function. Also the type would not match. Or am I missing something?
There was a problem hiding this comment.
Ah yes you're right. Ok then returning vector directly here also would be more than enough :).
vector<int> peaks_after_stim_start(const double stimstartindex,
const vector<int>& peakindices){
vector<int> newpeakindices;
for (size_t i = 0; i < peakindices.size(); i++) {
if (peakindices[i] > stimstartindex) {
newpeakindices.push_back(peakindices[i]);
}
}
return newpeakindices;
}
There was a problem hiding this comment.
done in latest commit
AurelienJaquier
changed the title
|
changelog and tests done in latest commits |
Pull Request Template
Fixes a few of AP_begin_indices dependent feature to make them behave like AP_begin_indices, i.e. do not take into account spikes before stim_start, but take into account spikes after stim_end.
Features changed: AP_end_indices, AP_rise_time, AP_fall_time, AP_rise_rate, AP_fall_rate
Also fixes other features dependent on them, like AP_duration
Description
Please include a summary of the change.
By default the change will increment the patch version.
To increment major or minor changes add #major or #minor to the PR description.
Checklist: