Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 matlab_strict API for matching MATLAB PREP's correlations and medians during RANSAC #70
Add matlab_strict API for matching MATLAB PREP's correlations and medians during RANSAC #70
Changes from all commits
ade66bb
a7a5f65
558108e
cb2a182
ef6c104
95bab50
6927d40
3670e3c
71691c0
ebf3b08
f3d3848
ace3d67
103fba1
954a03b
d3942a3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure whether or not this should be made public (including the import in init.py) --> for
NoisyChannels
, I can see why people may want to use it separately fromPrepPipeline
--> but do such usecases also exist for a barebonesReference
?A general rule is to expose as little API as possible ... and only as much as necessary :) That helps tremendously with development (backward incompatible changes) and also helps users to get a clear picture of what can be done and the reasoning behind it (not everybody is a power user).
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fully understood about wanting to keep the public API minimal. My personal use-case for
Reference
being used separately fromPrepPipreline
(apart from my MATLAB comparison efforts where I've pre-cleanlined the data) is that I want to be able to plot the results of Cleanline separately from the results of full PREP (cleanline + re-reference) in order to sanity-check the process at each step for each file (see this chunk of my script here).Apart from that use-case, the other main one I can think of would be if someone wanted to use a different line noise filtering method but still wanted to use PyPREP's robust re-referencing. That said, I guess you could get all the same functionality by restructuring
PrepPipeline
a bit to allow for separate CleanLine and/or re-referencing from that class instead of makingReference
public. If that sounds better to you (or if you'd just prefer to keep the public API as-is), I'll just removeReference
from the init along with my references to it in any public docstrings.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay cool, that sounds like a fair use. +1 to making it part of the public API then, as proposed here. In the future we should then add an example of how to make use of that (like you do in your script).