Feature calculators touchups #1103
Conversation
Scott-Simmons
force-pushed
the
feature-calculators-touchups
branch
2 times, most recently
from
d4d1862 to
5870185
Compare
| else: | ||
| return np.nan | ||
| avg = np.mean(x) | ||
| if avg != 0: |
There was a problem hiding this comment.
As you are already touching it...
It would probably be nicer to have the edge case as the if branch, so just switching the order.
| @@ -1099,23 +1098,21 @@ def fft_coefficient(x, param): | |||
| def complex_agg(x, agg): | |||
| if agg == "real": | |||
| return x.real | |||
| elif agg == "imag": | |||
| if agg == "imag": | |||
There was a problem hiding this comment.
I know that writing an else after a return does not make sense, but honestly I think it makes it clearer what is meant. If you have strong feelings about it, keep the if, but if not, maybe revert it :)
| return np.angle(x, deg=True) | ||
| return None |
There was a problem hiding this comment.
Good point. Maybe instead of returning None one should raise an exception because this should not happen?
| skew=get_skew, | ||
| kurtosis=get_kurtosis, | ||
| ) | ||
| return ( |
There was a problem hiding this comment.
As you are touching it anyways ;)
Can you extract a variable for the y centroid? It only needs to be calculated once
|
|
||
|
|
||
| @set_property("fctype", "simple") | ||
| def range_count(x, min, max): | ||
| def range_count(x, min_val, max_val): |
There was a problem hiding this comment.
I see your intention: min and max are python keywords, so they should not be used. But the config of the feature extractors is part of our API and changing it is in principle a breaking change, which we should be careful about.
How about keeping the function parameters the same but renaming it internally so that we do nit have any potential of mixup?
There was a problem hiding this comment.
API and changing it is in principle a breaking change,
Yep absolutely see your point - I dont think worth making a breaking change over this - will amend
There was a problem hiding this comment.
Reverted 1e42930
Changing variable names because this overloads the builtin python functions `max` and `min`. Best to rename these to reduce any risk
Use list(dict.items())
Reverts changes to the argument names for range count, see blue-yonder@3e3f936#r1915531370
- Extract out centroid as a var - Raise exception and add Literal type sig - Switch order of branch evaluation
Scott-Simmons
force-pushed
the
feature-calculators-touchups
branch
from
987d677 to
5b26355
Compare
Related commit: blue-yonder@776ef3d
No worries! Some remaining f-strings were added in this extra commit: 4bf916d |
Various touchups to the feature calculators.
The changes here are intended to improve code clarity by refactoring a few variables with more meaningful names and simplifying complex expressions.
Easiest to go commit by commit.