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

pyprep needs to be split into smaller, testable chunks #23

Open
sappelhoff opened this issue Apr 8, 2020 · 4 comments
Open

pyprep needs to be split into smaller, testable chunks #23

sappelhoff opened this issue Apr 8, 2020 · 4 comments
Labels
Milestone

Comments

@sappelhoff
Copy link
Owner

we need to refactor much of the code to be in smaller chunks (functions, preferably) that can be properly tested with pytest.

that'd make everyone more comfortable when we change pyprep behavior, like suggested in #19

@yjmantilla
Copy link
Collaborator

Do you mean functions as in don't overuse classes? In that regard I agree. Sometimes they add a layer of abstraction that could be avoided.

Nevertheless because we have a lot of parameters it sometimes requires passing a lot of arguments (or a dictionary) to the functions and it gets a little bit ugly to the eyes.

In our particular case I would think about making the find_noisy_channels criteria methods into functions of the module that the Noisy Channels class calls. That way they would be less dependent of each other and could be tested in an easier way (and potentially be used in other contexts?). Right now though the test file of find_noisy_channels doesn't seem that convoluted so maybe this is unnecesary (?).

The other two critical things are the perform_reference and the robust_reference methods. Maybe it would be worth while to make deeper tests for these ones whether we refactor or not into a more functional style.

@a-hurst
Copy link
Collaborator

a-hurst commented Jun 6, 2021

@sappelhoff With @yjmantilla's functional RANSAC and the NoisyChannels tests being broken into simpler chunks in #84, what do you think is left for this issue? Factoring out adaptive line noise filtering and post-rereference interpolation as discussed in #73 comes to mind (would be necessary for writing MATLAB-equivalence tests for either), but is there anything else you can think of?

@sappelhoff
Copy link
Owner Author

Factoring out adaptive line noise filtering and post-rereference interpolation as discussed in #73 comes to mind (would be necessary for writing MATLAB-equivalence tests for either)

+1, but apart from that, I think your recent efforts have left us well covered, especially with the matprep CI :-)

I think this one can be closed once #73 is closed.

@yjmantilla
Copy link
Collaborator

I agree with both, with #73 this should be closed.

@sappelhoff sappelhoff added this to the 0.5.0 milestone Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants