Skip to content

Remove peakdet CLI and scripts#45

Open
smoia wants to merge 3 commits into
physiopy:masterfrom
smoia:ref/remove_CLI
Open

Remove peakdet CLI and scripts#45
smoia wants to merge 3 commits into
physiopy:masterfrom
smoia:ref/remove_CLI

Conversation

@smoia

@smoia smoia commented Mar 11, 2021

Copy link
Copy Markdown
Member

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

smoia commented Mar 12, 2021

Copy link
Copy Markdown
Member Author

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

@62442katieb

Copy link
Copy Markdown

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

@smoia

smoia commented Mar 15, 2021

Copy link
Copy Markdown
Member Author

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

tsalo commented Mar 15, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown

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

smoia commented Mar 16, 2021

Copy link
Copy Markdown
Member Author

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

@rmarkello

Copy link
Copy Markdown
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

smoia commented Mar 17, 2021

Copy link
Copy Markdown
Member Author

Ah, thank you @rmarkello!

@tsalo

tsalo commented Mar 17, 2021

Copy link
Copy Markdown
Member

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

smoia commented Mar 17, 2021

Copy link
Copy Markdown
Member Author

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
Copy Markdown

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