Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove dependency on scikit-learn #321

Merged
merged 6 commits into from
Sep 13, 2021
Merged

Conversation

hofaflo
Copy link
Contributor

@hofaflo hofaflo commented Aug 22, 2021

As far as I can tell, scikit-learn is only used in the module processing.qrs. The two uses of the imported function normalize could be handled by a simple implementation in wfdb. This would allow wfdb to remove scikit-learn as a requirement and speed up the import time of wfdb.processing.

@bemoody
Copy link
Collaborator

bemoody commented Aug 23, 2021

Thanks; this looks good to me.

It looks to me like the existing code in XQRS._is_twave was buggy (wfdb/processing/qrs.py, lines 531-545). If I understand correctly, scipy.preprocessing.normalize would return an Nx1 array, np.diff would return an Nx0 array, the < comparison would return an empty one-dimensional array, so the function would always return false.

I would assume that the new version is in fact what @cx1111 originally intended, and hopefully should improve the overall specificity of the algorithm. But, Chen, do you think this is okay or should be tested more?

@bemoody
Copy link
Collaborator

bemoody commented Sep 9, 2021

For the old version of XQRS, over mitdb:

  • Gross QRS sensitivity: 99.35
  • Gross QRS positive predictivity: 99.74
  • Average QRS sensitivity: 99.34
  • Average QRS positive predictivity: 99.71

For the new version:

  • Gross QRS sensitivity: 99.03
  • Gross QRS positive predictivity: 99.76
  • Average QRS sensitivity: 99.08
  • Average QRS positive predictivity: 99.73

(Total false positives decreased from 237 to 216, false negatives increased from 592 to 888.)

Not great but not terrible, and might be useful sometimes.

The old behavior is equivalent to setting the config parameter t_inspect_period to zero:

qrs_inds = xqrs_detect(sig, fs, conf=XQRS.Conf(t_inspect_period=0))

So if we wanted to keep the old behavior by default we could just set the default t_inspect_period to 0.

@tompollard
Copy link
Member

Thanks @hofaflo. Nice checks @bemoody and I think it would make sense to set the default as you suggest. Please could someone take a look at doing this? Once the default is set, it sounds like we should be good to merge.

@hofaflo
Copy link
Contributor Author

hofaflo commented Sep 12, 2021

Thanks for the benchmark @bemoody!
Done @tompollard.

@tompollard
Copy link
Member

thanks @hofaflo, looks good to me! okay to merge @bemoody?

@bemoody
Copy link
Collaborator

bemoody commented Sep 13, 2021 via email

@tompollard
Copy link
Member

Thanks!

@tompollard tompollard merged commit b9c61aa into MIT-LCP:master Sep 13, 2021
@hofaflo hofaflo deleted the no-sklearn branch September 14, 2021 08:41
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants