Accelerated Visibility-Graph-Based Peak detector

[taulokoka]

Dear NeuroKit,

the visibility graph method you have included in your package (koka2022) has been recently updated (https://ieeexplore.ieee.org/document/10290007) and is being maintained at https://github.com/JonasEmrich/vg-beat-detectors. It would be appreciated, if you could update the method since the recent version is orders of magnitude faster than the previous, while providing better results.

Best regards,
Taulant Koka

[welcome]

Hi 👋 Thanks for reaching out and opening your first issue here! We'll try to come back to you as soon as possible. ❤️

[DominiqueMakowski]

Hi @JonasEmrich, there is a request for adding VG-based peak detection methods to NeuroKit, and I was wondering what your thoughts on this are ☺️ Options include:

1. Doing nothing
   • Cons: No potential visibility gains (methods included in larger libraries like NK tend to be discovered more, and thus used and cited
2. Soft dependency: calling the vg-method from NK would call vg-beat-detectors, but since we operate on a minimum dependency, it would be a soft dependency, i.e., asking users to manually install the package the first time.
   • Pros: no duplicated code, only one place to maintain
   • Cons: More prone to potential breaks (as changes your end can break things upstream). + many users cannot necessarily install smaller packages used for a couple of functions (e.g., in production contexts). That point likely doesn't apply as even with the other solutions below we would still need a conditional dependency on ts2vg
3. Duplicated code (code giveaway): You agree for your code to be copied over and eventually adapted
   • Cons: duplicated code. Increased maintenance burden for non-experts (on NK side)
4. Duplicated code (preserved maintainership): only difference is that you become a contributor of the adaptation/implementation of the method in NK and can help us out with maintenance
   • Pros: you are still responsible for the code, you become part of the NK team. More stable (i.e., you can still make quick updates to your library without fears of breaking NK, which can use a more stable/slow paced updating process)
   • Cons: duplicated code to maintain
5. Full integration into NK
   • Cons: less visibility for your work as it becomes part of NK

I'd say that option 4 is one of the best in terms of pros/cons balance, but let us know what you think!

[JonasEmrich]

Hi @DominiqueMakowski, thanks for reaching out!
I also think option 4 would be the optimal choice. Since I am fairly new to the process, maybe you could guide a bit me through the next steps 😃

[DominiqueMakowski]

Of course :)

The rough steps would be:

1. Fork NeuroKit
2. Put yourself on the dev branch
3. From there, create a new branch called e.g., ecg_peaks_vgmethod
4. I usually start by making a minimal change, e.g., add a comment on the ecg_findpeaks.py file
5. Commit this small change
6. Since your branch is now ahead of dev, you can open a PR (to the dev branch)
7. While the PR is open, we can then start working on the implementation (working on an open PR makes it easy as we can both make commits to it and review/make comments etc)
8. The core code of the method will go in ecg_findpeaks.py as an anonymous function (e.g., _ecg_findpeaks_visibilitygraph() (I'd say let's focus on the FastWHVG method as you seem to think it's superior?)
9. Then it needs to be activated in the ecg_findpeaks() function
10. Finally, it can be documented and referenced in the main user-facing function, ecg_peaks

Don't hesitate to add links to your package, for instance in the core code, like # Original function developed by ... and available in ... etc.

Don't hesitate to ask if there's anything

[stale]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.