Skip to content
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

fix AHP_slow_time and min_AHP_values #394

Merged
merged 7 commits into from
May 21, 2024

Conversation

AurelienJaquier
Copy link
Collaborator

Description

min_AHP_values and AHP_slow_time are a little bit special because they are computed during other features computation (min_AHP_indices and AHP_depth_abs_slow). So when we call them, the feature function does nothing, except calling the other feature with the dependency graph. The problem was that the 'empty' feature function still returns -1, and that can be caught afterwards, and replace the feature values with None (depending on the order of the features passed to efel).

This PR just replaces these -1 with 1s. The feature is no longer replaced by None. In the case where the feature cannot be computed but still returns 1, this does not cause a problem, because when pyfeatures.cppfeature_access.get_cpp_feature will not find the feature, it will just replace it with None, as expected.

Checklist:

  • Unit tests are added to cover the changes (skip if not applicable).
  • The changes are mentioned in the documentation (skip if not applicable).
  • CHANGELOG file is updated (skip if not applicable).

@AurelienJaquier AurelienJaquier self-assigned this May 21, 2024
@AurelienJaquier
Copy link
Collaborator Author

replace empty list features with None, following what is described here: Issue #321

if (vec.empty()) GErrorStr += "Feature [" + strName + "] data is missing\n";
if (vec.empty()) {
GErrorStr += "Feature [" + strName + "] data is missing\n";
return -1;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will turn empty lists into None

@codecov-commenter
Copy link

codecov-commenter commented May 21, 2024

Codecov Report

Attention: Patch coverage is 54.28571% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 91.78%. Comparing base (2cddd26) to head (e1f5966).
Report is 17 commits behind head on master.

Files Patch % Lines
efel/cppcore/SpikeEvent.cpp 44.44% 5 Missing and 5 partials ⚠️
efel/cppcore/SpikeShape.cpp 50.00% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #394      +/-   ##
==========================================
+ Coverage   91.73%   91.78%   +0.05%     
==========================================
  Files          36       37       +1     
  Lines        6278     7096     +818     
  Branches     2033     2276     +243     
==========================================
+ Hits         5759     6513     +754     
- Misses        266      304      +38     
- Partials      253      279      +26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ilkilic
Copy link
Collaborator

ilkilic commented May 21, 2024

wouldn't be clearer to call the dependent feature by using getFeatures and return their returning values instead?

@AurelienJaquier
Copy link
Collaborator Author

How would we do that concretely?

@ilkilic
Copy link
Collaborator

ilkilic commented May 21, 2024

would something like this make sense? (for min_AHP_values)

const auto& intFeatures = getFeatures(IntFeatureData, {"min_AHP_indices"});
auto retval = intFeatures.at("min_AHP_indices").size();
if (retval <= 0) {
    return -1;
} else {
    return retval;
}

@AurelienJaquier
Copy link
Collaborator Author

Ok I see, it makes sense. I'll implement it, thanks

@AurelienJaquier
Copy link
Collaborator Author

@ilkilic done in latest commit!

Copy link
Collaborator

@ilkilic ilkilic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic! Thank you!

@AurelienJaquier AurelienJaquier merged commit 18f7723 into BlueBrain:master May 21, 2024
20 checks passed
@AurelienJaquier AurelienJaquier deleted the fix-slowtime branch May 21, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants