-
Notifications
You must be signed in to change notification settings - Fork 55
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
Allow using lists of kwargs in morph_stats features #1021
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1021 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 36 36
Lines 2401 2403 +2
=========================================
+ Hits 2401 2403 +2 |
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.
Nice, thanks for this!
Either a list of features: [feature_name, {kwargs: {}, modes: []}] or | ||
a dictionary of features {feature_name: {kwargs: {}, modes: []}}. | ||
- kwargs is an optional entry allowing to pass kwargs to the feature function | ||
- modes is an aggregation operation provided as a string such as: |
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 want to keep both formats? Shouldn't we raise a deprecation warning for the dict format?
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.
Not sure where we stand wrt to deprecating the old formats. @mgeplf , should I add warnings for all previous config layouts?
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.
Hmm, the way I was thinking about adding this was to make, the kwargs
(which was poorly named), a list:
neurite:
partition_asymmetry:
kwargs:
- {}
- { variant: 'length', method: 'petilla' }
and then isinstance
kwargs to see if it was a dict (and then use the old behavior), or a list(and then use the new behavior. I think the config could be more minimal this way; however, the modes would be the same for both kwargs
variants.
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 like this. I will change it accordingly!
@lidakanari , @adrien-berchet , that would work for you right?
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 have implemented @mgeplf suggestion for multiple kwargs. @lidakanari , @adrien-berchet could you please try it and let me know if it works for you?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.
LGTM; but let's wait for the feedback on usage.
Yep, that works well for me, thanks! |
Looks perfect, thank you very much! |
Summary:
f'{mode}_{feature_name}__{suffix}'
)neurite_type
, e.g.variant:branch-order__method:uylings
extract_stats
,extract_dataframe
, andmain
.extract_stats
to guarantee that regardless of the entry point in the module, i.e. using any of the three functions in the previous point, the config is sanitized the same way.A list of features looks like this:
Using that
extract_stats
results to:Notice how the multiple feature entries with different kwargs result into different name:
Closes #1015