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

Add: slope_sign_change, log_detector and willison_amplitude #826

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nelsoncardenas
Copy link

@nelsoncardenas nelsoncardenas commented Mar 16, 2021

Features : slope_sign_change, log_detector and willison_amplitude.

This features are popular and useful in time domain analisys for EMG signals. The main reference for this features are: [1] Phinyomark, A., Phukpattaranont, P., & Limsakul, C. (2012). Feature reduction and selection for EMG signal classification. Expert systems with applications, 39(8), 7420-7431.

I'm a newbie to contributing to projects on Github, but I tried to follow the steps in your documentation.

Features : slope_sign_change, log_detector and willison_amplitude.

This features are popular and useful in time domain analisys for EMG signals. The main reference for this features are: [1] Phinyomark, A., Phukpattaranont, P., & Limsakul, C. (2012). Feature reduction and selection for EMG signal classification. Expert systems with applications, 39(8), 7420-7431.
# Calculation of feature as float
x = np.asarray(x)
n = len(x)
x = x + (x == 0)*0.000000000001
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the motivation behind this line?

Copy link
Author

@nelsoncardenas nelsoncardenas May 18, 2021

Choose a reason for hiding this comment

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

I investigated it better and I think what I did is not a good strategy. The idea was to avoid errors generated by values equal to zero for the next line but we use a low value, and this tends to infinity.

| [1] Phinyomark, A., Phukpattaranont, P., & Limsakul, C. (2012). Feature reduction and selection for EMG signal classification. Expert systems with applications, 39(8), 7420-7431.
"""
# Calculation of feature as int
return np.sum(np.abs(x[1:] - x[:-1]) > t)
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of x[1:] - x[:-1] I think you could use np.diff

Copy link
Collaborator

@MaxBenChrist MaxBenChrist left a comment

Choose a reason for hiding this comment

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

can you add some unit tests to this pr?

@nils-braun
Copy link
Collaborator

Great, @theinem ! Three really interesting features. As @MaxBenChrist mentioned, could you add some unittests for your new feature calculators? We try to test every calculator :-)
Also, you new calculators need settings, so you need to add them to the long list in settings.py to the ComprehensiveFCParameters (that will also solve your test failures).
Could you also fix the python style issues?

@nelsoncardenas
Copy link
Author

As I said in my other PR, I would like to make the improvements you asked for, but for several weeks I have had problems with available time and it will continue like this for several weeks/months.

@nils-braun
Copy link
Collaborator

Thanks for the info, @theinem. As mentioned in #825, this is not a problem for us!

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