update C++ codebase for handling of feature dependencies [vcs: #minor] #334

anilbey · 2023-11-29T10:52:37Z

Changes

Following up on #321, focusing on the handling of feature dependencies.

getFeatures Function for Enhanced Dependency Handling

Consistent Result Differentiation: The updated getFeatures now consistently distinguishes between empty results and actual failures.
Restricted Access for Safety: The introduction of constant (const) access modifiers in the new implementation restricts unnecessary access to feature data, preventing unintended changes.
Centralized Exception and Missing Value Handling: Exceptions and missing values are now handled once for all features in a unified manner, replacing the previous approach where each feature managed these issues separately. This consistency streamlines error handling and arguably improves code readability.

Before

int LibV2::AP_rise_time(mapStr2intVec& IntFeatureData,
                        mapStr2doubleVec& DoubleFeatureData,
                        mapStr2Str& StringData) {
  int retval;
  vector<double> t;
  retval = getVec(DoubleFeatureData, StringData, "T", t);
  if (retval < 0) return -1;
  vector<int> apbeginindices;
  retval = getVec(IntFeatureData, StringData, "AP_begin_indices",
                     apbeginindices);
  if (retval < 0) return -1;
  vector<int> peakindices;
  retval = getVec(IntFeatureData, StringData, "peak_indices",
                     peakindices);
  if (retval < 0) return -1;
  vector<double> v;
  retval = getVec(DoubleFeatureData, StringData, "V", v);
  if (retval < 0) return -1;
  vector<double> AP_amplitude;
  retval =
      getVec(DoubleFeatureData, StringData, "AP_amplitude", AP_amplitude);
  if (retval < 0) {
    GErrorStr += "Error calculating AP_amplitude for mean_AP_amplitude";
    return -1;
  } else if (retval == 0) {
    GErrorStr += "No spikes found when calculating mean_AP_amplitude";
    return -1;
  } else if (AP_amplitude.size() == 0) {
    GErrorStr += "No spikes found when calculating mean_AP_amplitude";
    return -1;
  }
  // Get rise begin percentage
  vector<double> risebeginperc;
  retval = getVec(DoubleFeatureData, StringData, "rise_start_perc", risebeginperc);
  if (retval <= 0) {
    risebeginperc.push_back(0.0);
  }
  // Get rise end percentage
  vector<double> riseendperc;
  retval = getVec(DoubleFeatureData, StringData, "rise_end_perc", riseendperc);
  if (retval <= 0) {
    riseendperc.push_back(1.0);
  }
  vector<double> aprisetime;
  retval = __AP_rise_time(t, v, apbeginindices, peakindices, AP_amplitude, risebeginperc[0], riseendperc[0], aprisetime);
  if (retval >= 0) {
    setVec(DoubleFeatureData, StringData, "AP_rise_time", aprisetime);
  }
  return retval;
}

After

int LibV2::AP_rise_time(mapStr2intVec& IntFeatureData,
                        mapStr2doubleVec& DoubleFeatureData,
                        mapStr2Str& StringData) {
  // Fetching all required features in one go.
  const auto& doubleFeatures = getFeatures(DoubleFeatureData, 
                                    {"T", "V", "AP_amplitude", "rise_start_perc", "rise_end_perc"});
  const auto& intFeatures = getFeatures(IntFeatureData, 
                                 {"AP_begin_indices", "peak_indices"});
  vector<double> aprisetime;
  int retval = __AP_rise_time(
      doubleFeatures.at("T"),
      doubleFeatures.at("V"),
      intFeatures.at("AP_begin_indices"),
      intFeatures.at("peak_indices"),
      doubleFeatures.at("AP_amplitude"),
      doubleFeatures.at("rise_start_perc").empty() ? 0.0 : doubleFeatures.at("rise_start_perc").front(),
      doubleFeatures.at("rise_end_perc").empty() ? 1.0 : doubleFeatures.at("rise_end_perc").front(),
      aprisetime
  );
  if (retval > 0) {
    setVec(DoubleFeatureData, StringData, "AP_rise_time", aprisetime);
  }
  return retval;
}

Simplifying Variable Handling

Eliminating Unnecessary Wildcards

Context: In our C++ code, wildcards lose their relevance when used with Python, as Python's variables naturally offer the flexibility wildcards are meant to provide.
Advantage: Removing these wildcards makes our code leaner and more efficient, as Python itself can accomplish these tasks without needing additional C++ scripting.
Impact: This alteration has no negative effect on downstream applications like bluepyefe and simplifies feature representation in cfeature, DependencyTree, and mapoperations.
Example: The example case is the use of "location_AIS" as a wildcard in C++. Such explicit use is redundant when we interface with Python.

 int LibV5::AP_phaseslope_AIS(mapStr2intVec& IntFeatureData,
                             mapStr2doubleVec& DoubleFeatureData,
                             mapStr2Str& StringData) {
  int retVal;
  vector<double> ap_phaseslopes;
  retVal = getVec(DoubleFeatureData, StringData,
                        "AP_phaseslope;location_AIS", ap_phaseslopes);
  if (retVal < 0) return -1;
  setVec(DoubleFeatureData, StringData, "AP_phaseslope_AIS",
               ap_phaseslopes);
  return retVal;
}

Merging efel.h/efel.cpp into cppcore

Reason: These two files offer similar functionalities but are maintained separately, which is inefficient.
Benefit: Merging them reduces the number of files to compile and manage, thereby making our codebase more efficient and easier to navigate.

Embracing Flexibility with Templates

Update: Substitute specific functions like getIntParam and getDoubleParam with a unified getParam.
Outcome: This change prevents code duplication and allows our system to handle different data types more effectively.

Update:

14.12.2023

Added more clarity on the removal of E* features by quoting @darshanmandge .

" Feature starting with E such as E6, here seem to be duplicates of existing features with wildcard for APWaveform."

The names of those features are as follows: "E6 E7 E39 E39_cod E2 E3 E4 E5 E8 E9 E10 E11 E12 E13 E14 E15 E16 E17 E18 E19 E20 E21 E22 E23 E24 E25 E26 E27 E40"

08.01.2024

bpap_attenuation feature is added to the Python API.
Spikecount, Spikecount_stimint, burst_number and strict_burst_number features migrated to Python from C++.
check_ais_initiation is added to the Python API.

This reverts commit 954613a.

anilbey · 2024-01-08T09:55:52Z

@darshanmandge, @AurelienJaquier I tried to address all of your reviews. Could you take another look to ensure the modifications align with your expectations?

AurelienJaquier · 2024-01-08T10:13:14Z

efel/pyfeatures/pyfeatures.py

+    return spike_count()
+
+
+def spike_count() -> numpy.ndarray:


Is there a reason to have a new spike_count feature and deprecating Spikecount? Also since it stays as Spikecount in the documentation.
Same question for Spikecount_stimint

In the future I thought keeping Python naming convention would be better. I wanted to deprecate it now so that if we change the naming convention in the future we can say: this was deprecated 2 major releases ago.

Ideally all features should have a consistent naming. Most of them are underscore separated already e.g.
adaptation_index or ohmic_input_resistance. Spikecount was an exception. What do you think?

Alright, sounds good. But then maybe we should update the documentation also.

I see Spikecount has been used in several places in BluePyOpt and BluePyEModel. There can be lot of warnings from old code at multiple places as it is one of the most common features.

Do all the other features follow consistent naming convention? If not, can we keep the name and feature Spikecount ?

Ok. Thanks!

While testing bluepyopt, bluepyefe, bluepyemodel I will make sure this warning does not occur multiple times.

Shouldn't you add spike_count in all_pyfeatures to make it a valid feature? If you want to deprecate Spikecount I mean. Also modify documentation so that feature name is spike_count while keeping a note for Spikecount explaining that it is deprecated.

I think "spike_count" is there already.

Updating the docs

darshanmandge · 2024-01-08T13:10:51Z

@darshanmandge, @AurelienJaquier I tried to address all of your reviews. Could you take another look to ensure the modifications align with your expectations?

Before merging this PR, can you check if works correctly with BluePyOpt, BluePyEfe and BluePyEModel e.g. the tests and examples? This would be another level of check to ensure eFEL works well with these software. :)

anilbey · 2024-01-08T13:48:34Z

Downstream workflows to be tested before merging this PR:

bluepyefe
bluepyopt
bluepyemodel

bluepyefe

py3: OK (56.08=setup[49.34]+cmd[6.75] seconds)
congratulations :) (56.42 seconds)

bluepyopt

==================================================================== 180 passed, 12 deselected, 16 warnings in 19.54s =====================================================================
===================================================================== 11 passed, 181 deselected in 390.37s (0:06:30) ======================================================================
====================================================================== 1 passed, 191 deselected in 215.89s (0:03:35) ======================================================================
py3-unit-functional-style: OK (740.07=setup[76.88]+cmd[7.10,1.13,20.28,27.37,390.92,216.40] seconds)

bluepyemodel

========================================================================= 75 passed, 52 warnings in 176.40s (0:02:56) ==========================================================================

This reverts commit 41b087e.

AurelienJaquier · 2024-01-09T16:32:47Z

tests/test_basic.py

@@ -1711,6 +1711,8 @@ def test_getFeatureNames():
    test_data_path = testdata_dir.parent / 'featurenames.json'
    with open(test_data_path, 'r') as featurenames_json:
        expected_featurenames = json.load(featurenames_json)
+    # add the new names for the deprecated ones
+    expected_featurenames += ["spike_count", "spike_count_stimint"]


The featurenames_json should reflect all the features that we have. If we don't want to duplicate spike_count, it would be better I think to have spike_count in the file, and add deprecated Spikecount in code here to legacy, instead of the inverse as we have now

Ok makes sense so renaming Spikecount -> spike_count in the file.

Done in the last push.

AurelienJaquier · 2024-01-09T16:33:02Z

tests/test_cppcore.py

@@ -97,6 +97,8 @@ def test_getFeatureNames(self):  # pylint: disable=R0201
        test_data_path = os.path.join(testdata_dir, '../featurenames.json')
        with open(test_data_path, 'r') as featurenames_json:
            expected_featurenames = json.load(featurenames_json)
+        # add the new names for the deprecated ones
+        expected_featurenames += ["spike_count", "spike_count_stimint"]


Same as comment above

Updated in the latest push.

AurelienJaquier · 2024-01-10T08:14:00Z

docs/source/eFeatures.rst


 number of spikes in the trace, including outside of stimulus interval

 - **Required features**: LibV1:peak_indices
 - **Units**: constant
 - **Pseudocode**: ::

-    Spikecount = len(peak_indices)
+    spike_count = len(peak_indices)

 **Note**: In the future this feature will be called "spike_count".


Could you please tell in the note that this feature replaces Spikecount, that Spikecount is deprecated, but can still be used for the moment?

Ah yes, I also noticed I forgot to update this one. Done in the last commit. 👍

AurelienJaquier

Great job!

anilbey added 30 commits October 20, 2023 16:36

throw runtime_error, rm exit(1) in featuretype&calcfeatures

0e6dbcd

rename type->input_type

5cc3c5e

throw EfelAssertionError to avoid exit(-1)

2a84582

add test to trigger C++->Python AssertionError

84626df

remove featurename.find(";") check

954613a

typo in docstring

e9cbcc8

add feature's name to the error message

443e083

Revert "remove featurename.find(";") check"

f11e36d

This reverts commit 954613a.

remove setversion function

108ca85

draft: removing alternative wildcard syntax

71f687d

merge master

2ba460d

remove variable features in LibV5

3748628

remove variable (alias) features in LibV2

9fa5fa4

simplify feature pointers representation in cppcore, remove wildcards

f07895b

make AddUniqueItem void

655cd03

remove empty function getDependencyList

34cfb4d

directly check stream's state after opening the file

b8f629a

remove dead code in efel and cfeature

d91d476

merge efel into cppcore

9172cc7

add template to getParam

8ed52a8

add getFeatures fn to get all dependent features

786f636

LibV5.cpp update until time_to_last_spike

6a80aeb

libv5 update ISI computations

cf52d1d

LibV5 remove ISI first, second etc. duplication

1abfbac

calculateInvISI throw except instead of return 0

32eb094

use getFeatures in depolarized_base

13c21f6

use getFeatures in steady_state_hyper

9ad88fd

depolarized_based consider retVal==0 failure

4de7876

use getFeatures in LibV2

3c86f4e

use getFeatures in LibV1

4d36018

anilbey changed the title ~~Modernising C++ codebase for handling of feature dependencies~~ update C++ codebase for handling of feature dependencies #minor Jan 8, 2024

AurelienJaquier reviewed Jan 8, 2024

View reviewed changes

anilbey added 2 commits January 8, 2024 14:46

docs: add multitrace and validation modules to API

cc4f705

Merge branch 'master' into wildcards

c02f4c9

anilbey added 11 commits January 8, 2024 14:54

Docs: add name change warnings

df224bd

Merge branch 'wildcards' of github.com:BlueBrain/eFEL into wildcards

c7ff80a

add int/double template instantiations to cfeature

082f205

remove getDistance_cpp, use python implementation

c7a4cc9

move trace_check to pyfeatures

5f8881b

lint fix

b6940d1

add from __future__ import annotations

87cc375

add stimulus_current to test_allfeatures_on_constant_V

41b087e

allow both spike_count and Spikecount

f39e7eb

move validation.py inside pyfeatures

489c247

Revert "add stimulus_current to test_allfeatures_on_constant_V"

c6033de

This reverts commit 41b087e.

AurelienJaquier reviewed Jan 9, 2024

View reviewed changes

anilbey added 2 commits January 9, 2024 17:33

add spike_count expect it to be 0 for allfeatures_on_constant_voltage

2682494

encourage use of spike_count instead of Spikecount

43bf8b3

AurelienJaquier reviewed Jan 10, 2024

View reviewed changes

update deprecation note in the docs

bc15caa

anilbey changed the title ~~update C++ codebase for handling of feature dependencies #minor~~ update C++ codebase for handling of feature dependencies [vcs: #minor] Jan 10, 2024

AurelienJaquier approved these changes Jan 10, 2024

View reviewed changes

anilbey merged commit cc9a2b7 into master Jan 10, 2024
8 checks passed

anilbey deleted the wildcards branch January 10, 2024 09:06

anilbey mentioned this pull request Jan 15, 2024

"number_initial_spikes" feature should return 0 when there are no spikes instead of None #341

Closed

AurelienJaquier mentioned this pull request May 27, 2024

fix edge case in AP_begin_indices #397

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update C++ codebase for handling of feature dependencies [vcs: #minor] #334

update C++ codebase for handling of feature dependencies [vcs: #minor] #334

anilbey commented Nov 29, 2023 •

edited

Loading

anilbey commented Jan 8, 2024

AurelienJaquier Jan 8, 2024

anilbey Jan 8, 2024

anilbey Jan 8, 2024

AurelienJaquier Jan 8, 2024

darshanmandge Jan 8, 2024

darshanmandge Jan 8, 2024

anilbey Jan 8, 2024 •

edited

Loading

AurelienJaquier Jan 9, 2024

anilbey Jan 9, 2024

anilbey Jan 9, 2024

darshanmandge commented Jan 8, 2024

anilbey commented Jan 8, 2024 •

edited

Loading

AurelienJaquier Jan 9, 2024

anilbey Jan 9, 2024

anilbey Jan 9, 2024

AurelienJaquier Jan 9, 2024

anilbey Jan 9, 2024

AurelienJaquier Jan 10, 2024

anilbey Jan 10, 2024

AurelienJaquier left a comment

update C++ codebase for handling of feature dependencies [vcs: #minor] #334

update C++ codebase for handling of feature dependencies [vcs: #minor] #334

Conversation

anilbey commented Nov 29, 2023 • edited Loading

Changes

getFeatures Function for Enhanced Dependency Handling

Before

After

Simplifying Variable Handling

Eliminating Unnecessary Wildcards

Merging efel.h/efel.cpp into cppcore

Embracing Flexibility with Templates

Update:

14.12.2023

08.01.2024

anilbey commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anilbey Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darshanmandge commented Jan 8, 2024

anilbey commented Jan 8, 2024 • edited Loading

bluepyefe

bluepyopt

bluepyemodel

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AurelienJaquier left a comment

Choose a reason for hiding this comment

anilbey commented Nov 29, 2023 •

edited

Loading

anilbey Jan 8, 2024 •

edited

Loading

anilbey commented Jan 8, 2024 •

edited

Loading