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

Add matlab_strict API for matching MATLAB PREP's correlations and medians during RANSAC #70

Merged
merged 15 commits into from
Apr 26, 2021

Conversation

a-hurst
Copy link
Collaborator

@a-hurst a-hurst commented Apr 23, 2021

PR Description

Closes #65.

This PR introduces a new argument for the main PyPREP functions/classes called matlab_strict, which overrides PyPREP's deliberate differences in defaults over MATLAB PREP to produce identilcal results. Support for MatPREP-style median calculation and correlation calculation during RANSAC have both been added and wrapped in this API.

I also added a draft of a documentation page explaining where PyPREP deviates intentionally from MatPREP and why, since I figured that would be a useful thing for users (and developers) to have. I should add references to it wherever matlab_strict is mentioned, but it's late here and I didn't want to wait another day before getting this up here for initial review.

Let me know what you think!

Merge Checklist

  • the PR has been reviewed and all comments are resolved
  • all CI checks pass
  • (if applicable): the PR description includes the phrase closes #<issue-number> to automatically close an issue
  • (if applicable): bug fixes, new features, or API changes are documented in whats_new.rst

@codecov-commenter
Copy link

codecov-commenter commented Apr 23, 2021

Codecov Report

Merging #70 (d3942a3) into master (6689155) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
+ Coverage   97.06%   97.15%   +0.08%     
==========================================
  Files           7        7              
  Lines         580      597      +17     
==========================================
+ Hits          563      580      +17     
  Misses         17       17              
Impacted Files Coverage Δ
pyprep/__init__.py 100.00% <100.00%> (ø)
pyprep/find_noisy_channels.py 96.53% <100.00%> (+0.02%) ⬆️
pyprep/prep_pipeline.py 100.00% <100.00%> (ø)
pyprep/ransac.py 97.34% <100.00%> (+0.07%) ⬆️
pyprep/reference.py 96.55% <100.00%> (+0.02%) ⬆️
pyprep/utils.py 98.61% <100.00%> (+0.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6689155...d3942a3. Read the comment docs.

Copy link
Owner

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I like the idea to explicitly document that.

I suggest you add the new RST page as a sidebar entry of the docs, similar to examples, and API currently:

pyprep/docs/conf.py

Lines 90 to 96 in 6689155

"navbar_links": [
("Examples", "auto_examples/index"),
("API", "api"),
("What's new", "whats_new"),
("GitHub", "https://github.com/sappelhoff/pyprep", True),
],
}

BTW, here is the rendered page: https://pyprep--70.org.readthedocs.build/en/70/matlab_differences.html

docs/matlab_differences.rst Outdated Show resolved Hide resolved
docs/matlab_differences.rst Show resolved Hide resolved
docs/matlab_differences.rst Outdated Show resolved Hide resolved
Copy link
Owner

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should add references to it wherever matlab_strict is mentioned

+1

Copy link
Owner

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Only the links are not yet working. See https://pyprep--70.org.readthedocs.build/en/70/matlab_differences.html#matlab-diffs

And try to click on some of the highlighted functions --> that should link to the API docs automatically, but it doesn't do that yet.

I think you need to include an RST "directive" in the RST file before making the first :func: (or similar) link. Something like the following (but might need tweaks)

.. currentmodule:: pyprep

see also for inspiration: https://raw.githubusercontent.com/sappelhoff/pyprep/master/docs/api.rst

@sappelhoff
Copy link
Owner

one more thing, looking at the author list in https://pyprep--70.org.readthedocs.build/en/70/whats_new.html

it's actually not alphabetical --> can you sort, please? :)

@sappelhoff sappelhoff added this to the 0.4.0 milestone Apr 24, 2021
@a-hurst
Copy link
Collaborator Author

a-hurst commented Apr 24, 2021

Only the links are not yet working. See https://pyprep--70.org.readthedocs.build/en/70/matlab_differences.html#matlab-diffs

And try to click on some of the highlighted functions --> that should link to the API docs automatically, but it doesn't do that yet.

This seem to be a problem across the docs: if you look at the links on the What's New and API pages, the only ones that seem to be working are for NoisyChannels and its methods, even though there's no obvious difference in formatting between the link for that and the one for PrepPipeline on the API page (which doesn't link properly).

I'm looking into it, will let you know what I figure out.

@sappelhoff
Copy link
Owner

Ahhh, it could be because for those links that don't work, there is just no api entry in the docs?

@a-hurst
Copy link
Collaborator Author

a-hurst commented Apr 24, 2021

Ahhh, it could be because for those links that don't work, there is just no api entry in the docs?

I figured it out: it's because the __init__.py exposes those classes/modules as pyprep.NoisyChannels and pyprep.PrepPipeline, etc. instead of pyprep.find_noisy_channels.NoisyChannels and pyprep.pipeline.PrepPipeline. I'm fixing this throughout the docs now, should be done shortly :)

Comment on lines +25 to +34
The :class:`~pyprep.Reference` class
------------------------------------

.. autosummary::
:toctree: generated/

Reference

The :class:`~pyprep.PrepPipeline` class
---------------------------------------
Copy link
Owner

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 from PrepPipeline --> but do such usecases also exist for a barebones Reference?

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?

Copy link
Collaborator Author

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 from PrepPipreline (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 making Reference public. If that sounds better to you (or if you'd just prefer to keep the public API as-is), I'll just remove Reference from the init along with my references to it in any public docstrings.

Copy link
Owner

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).

Copy link
Owner

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks well to me now and I think we have discussed all open points, right?

Please let me know @a-hurst --> if all is done from your side, then I'll merge this one tomorrow morning (or as soon as I hear back from you).

🚀

@a-hurst
Copy link
Collaborator Author

a-hurst commented Apr 25, 2021

@sappelhoff Yep, I think everything's been addressed! Looking forward to getting this merged and ticking off some more boxes in #58 🙂

@sappelhoff sappelhoff merged commit 0fa6ac4 into sappelhoff:master Apr 26, 2021
@sappelhoff
Copy link
Owner

Thanks!

@a-hurst a-hurst mentioned this pull request Apr 26, 2021
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RANSAC correlations (and logic) differ between PyPREP and MATLAB PREP
3 participants