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

Remove peakdet CLI and scripts #45

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

smoia
Copy link
Member

@smoia smoia commented Mar 11, 2021

Under @rmarkello's suggestions in #39, this PR removes the cli module from peakdet, as well as the script folder and the two dependencies related to them (Gooey and wxpython).

It depends on #39 - can't be merged before that PR.

@smoia smoia added MInormod-breaking For development only, this PR increments the minor version (0.+1.0) but breaks compatibility Paused Issue should not be worked on until the resolution of other issues or Pull Requests and removed Paused Issue should not be worked on until the resolution of other issues or Pull Requests labels Mar 11, 2021
@smoia
Copy link
Member Author

smoia commented Mar 12, 2021

This is ready for review (but also very simple)! @physiopy/peakdet

@62442katieb
Copy link

Previous comment was outdated, that's my bad. LGTM!

@smoia
Copy link
Member Author

smoia commented Mar 15, 2021

Good catch @62442katieb ! You were right - now it's fixed.

Also, since I'm already butchering the poor creature here, what do you, @tsalo, and @physiopy/phys2denoise think about the functions and classes in peakdet.modalities and peakdet.analytics? I'd say that we could remove them from here and add what is missing or makes sense to add to phys2denoise. WDYT?

@tsalo
Copy link
Member

tsalo commented Mar 15, 2021

At first glance, I think the code could be very useful in phys2denoise, but we would probably want to convert it to a functional approach to match phys2denoise's style.

@62442katieb
Copy link

Yes to peakdet.modalities, but the only thing in peakdet.analytics is heart rate variabilty computations. Are those used for denoising? (genuine question, as my knowledge of HR-based denoising has some gaps)

@smoia
Copy link
Member Author

smoia commented Mar 16, 2021

I genuinely don't know. In any case, I'll remove them - they're going to be in history.

@rmarkello
Copy link
Member

(No, generally HRV metrics are used in analysis, not processing / denoising. I'd say it's safe to remove them if you're focusing the module on just processing 👍)

@smoia
Copy link
Member Author

smoia commented Mar 17, 2021

Ah, thank you @rmarkello!

@tsalo
Copy link
Member

tsalo commented Mar 17, 2021

There probably should be a place within the physiopy umbrella for calculating metrics for analysis rather than denoising, right? Do we have a planned place for things like that?

@smoia
Copy link
Member Author

smoia commented Mar 17, 2021

If the analysis is MRI related, then phys2denoise would be the place for it (at least for the moment). If it's physiological signals only, probably other libraries would be more indicated (e.g. NeuroKit).

@CesarCaballeroGaudes
Copy link

Yes, HRV has only been proposed for analysis, not denoising.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MInormod-breaking For development only, this PR increments the minor version (0.+1.0) but breaks compatibility
Projects
Status: PR to review
Development

Successfully merging this pull request may close these issues.

5 participants