-
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
update C++ codebase for handling of feature dependencies [vcs: #minor] #334
Merged
Changes from 2 commits
Commits
Show all changes
94 commits
Select commit
Hold shift + click to select a range
0e6dbcd
throw runtime_error, rm exit(1) in featuretype&calcfeatures
anilbey 5cc3c5e
rename type->input_type
anilbey 2a84582
throw EfelAssertionError to avoid exit(-1)
anilbey 84626df
add test to trigger C++->Python AssertionError
anilbey 954613a
remove featurename.find(";") check
anilbey e9cbcc8
typo in docstring
anilbey 443e083
add feature's name to the error message
anilbey f11e36d
Revert "remove featurename.find(";") check"
anilbey 108ca85
remove setversion function
anilbey 71f687d
draft: removing alternative wildcard syntax
anilbey 2ba460d
merge master
anilbey 3748628
remove variable features in LibV5
anilbey 9fa5fa4
remove variable (alias) features in LibV2
anilbey f07895b
simplify feature pointers representation in cppcore, remove wildcards
anilbey 655cd03
make AddUniqueItem void
anilbey 34cfb4d
remove empty function getDependencyList
anilbey b8f629a
directly check stream's state after opening the file
anilbey d91d476
remove dead code in efel and cfeature
anilbey 9172cc7
merge efel into cppcore
anilbey 8ed52a8
add template to getParam
anilbey 786f636
add getFeatures fn to get all dependent features
anilbey 6a80aeb
LibV5.cpp update until time_to_last_spike
anilbey cf52d1d
libv5 update ISI computations
anilbey 1abfbac
LibV5 remove ISI first, second etc. duplication
anilbey 32eb094
calculateInvISI throw except instead of return 0
anilbey 13c21f6
use getFeatures in depolarized_base
anilbey 9ad88fd
use getFeatures in steady_state_hyper
anilbey 4de7876
depolarized_based consider retVal==0 failure
anilbey 3c86f4e
use getFeatures in LibV2
anilbey 4d36018
use getFeatures in LibV1
anilbey 65e8f60
using to replace scope resolution
anilbey d0084a3
define distinct errors: FeatureComputationError, EmptyFeatureError
anilbey 84b3020
use getFeature/s in LibV5 for consistent handling of edge cases
anilbey 841f2b3
remove unnecessary initializer_list
anilbey ffd9d03
min_AHP_indices to use getFeatures
anilbey 90a5dc6
AHP_depth_abs with getFeature
anilbey 749e002
spike_half_width to use getFeatures
anilbey 932b1b1
AP_begin_indices to use getFeatures
anilbey 35a8450
use getFeatures in current_base
anilbey 0510877
getfeatures in burst_begin_indices
anilbey 378c278
decay_time_constant_after_stim to use getFeatures
anilbey 8ded463
libv5 exception handling
anilbey a21b653
remove getVec completely
anilbey f3b3a73
Merge branch 'master' into wildcards
anilbey 4e6514b
make format
anilbey f7425b8
Merge branch 'master' into wildcards
anilbey d1101d9
replace throw EmptyFeatureError with return -1 for consistency
anilbey a4335ec
Merge branch 'wildcards' of https://github.com/BlueBrain/eFEL into wi…
anilbey bde76af
remove redundant else condition
anilbey d6848c3
avoid fetching ignore_first_ISI twice
anilbey 3ca2801
in the spikewidth2 error message mention spikewidth2
anilbey 9b9d382
update maximum_voltage's success case to be same asminimum_voltages
anilbey d5d5ed7
make peakvoltage vector a reference in amp_drop_first_last
anilbey e7907c8
remove obsolete empty feature checks in spike_width1
anilbey e790e16
remove else after throw
anilbey f3adcc3
update new line in code comment for readability
anilbey 83a57b1
replace GErrorStr+1 and return -1 via exception
anilbey 444bfe7
add test to cover single spike case in min_voltage_between_spikes
anilbey 5a5c22d
cover stimulus_current==0.0 edge case in ohmic resistance features
anilbey d7c576f
remove else after throw in 3 LibV5 places
anilbey 944140a
typo in test name
anilbey b3a33f3
test_ohmic_input_resistance_zero_stimulus_current update stim params
anilbey 1253dd3
update fail case for spikecount & spikecount_stimint
anilbey 5784c06
add test calling all features on constant trace
anilbey c101450
remove re imported import in test
anilbey 2ff86ed
merge master into wildcards
anilbey 267220d
impute Spikecount missing val to 0.0 in GranuleCellDeap1.ipynb
anilbey 9e9286f
remove unnecessary variable
anilbey 241a2d7
move Spikecount implementation to Python
anilbey 46007d2
change doc of Spikecount as Python efeature
anilbey 818e3a1
move Spikecount_stimint to Python
anilbey f778c6a
add validation module with check_ais_initiation
anilbey eebc16e
add bpap_attenuation as a multitrace feature
anilbey 31b7212
move burst_number and strict_burst_number to Python
anilbey 4c1aae3
update impedance, remove spikecount is None check
anilbey d287792
add CHANGELOG.md
anilbey 7a2594d
Merge branch 'master' into wildcards
anilbey 133a48e
Merge branch 'master' into wildcards
anilbey cc4f705
docs: add multitrace and validation modules to API
anilbey c02f4c9
Merge branch 'master' into wildcards
anilbey df224bd
Docs: add name change warnings
anilbey c7ff80a
Merge branch 'wildcards' of github.com:BlueBrain/eFEL into wildcards
anilbey 082f205
add int/double template instantiations to cfeature
anilbey c7a4cc9
remove getDistance_cpp, use python implementation
anilbey 5f8881b
move trace_check to pyfeatures
anilbey b6940d1
lint fix
anilbey 87cc375
add from __future__ import annotations
anilbey 41b087e
add stimulus_current to test_allfeatures_on_constant_V
anilbey f39e7eb
allow both spike_count and Spikecount
anilbey 489c247
move validation.py inside pyfeatures
anilbey c6033de
Revert "add stimulus_current to test_allfeatures_on_constant_V"
anilbey 2682494
add spike_count expect it to be 0 for allfeatures_on_constant_voltage
anilbey 43bf8b3
encourage use of spike_count instead of Spikecount
anilbey bc15caa
update deprecation note in the docs
anilbey File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is there a reason to have a new
spike_count
feature and deprecatingSpikecount
? Also since it stays asSpikecount
in the documentation.Same question for
Spikecount_stimint
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.
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
.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.
Ideally all features should have a consistent naming. Most of them are underscore separated already e.g.
adaptation_index
orohmic_input_resistance
.Spikecount
was an exception. What do you think?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.
Alright, sounds good. But then maybe we should update the documentation also.
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 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
?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.
Ok. Thanks!
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.
While testing bluepyopt, bluepyefe, bluepyemodel I will make sure this warning does not occur multiple times.
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.
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.
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 think "spike_count" is there already.
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.
Updating the docs