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

Functional ransac #51

Merged
merged 8 commits into from
Feb 6, 2021
Merged

Conversation

yjmantilla
Copy link
Collaborator

@yjmantilla yjmantilla commented Dec 10, 2020

PR Description

Made ransac methods into their function versions.
Then moved the functions into a standalone module called ransac.
Some changes were made to the parameters of the methods as a result.

Since I moved the stuff I dont know what to put in the RST for old entrances (ie :meth:NoisyChannels.run_ransac) and so on. This is now in :func:ransac.run_ransac. But we can move everything back to find_noisy_channels if that is a better way.

I also made the channel_correlations an output of the find_bad_by_ransac function for easier debugging, but I dont know if this is the best approach, so suggestions are welcome. It could also be a class that holds the end results (no mutable attributes), or maybe a dictionary.

Merge Checklist

To merge your PR we need to first take the following points into account:

  • 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

@sappelhoff
Copy link
Owner

@a-hurst can you offer your opinion here as well, please?

@yjmantilla
Copy link
Collaborator Author

yjmantilla commented Dec 13, 2020

Note that the idea of this code is to be able to do #36 , not to make the ultimate ransac function that in theory should arise from the comparison we make.

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 suggest to make all functions in ransac.py except find_bad_by_ransac private. That means putting an underscore _ in front of the function names.

Furthermore, the new module needs to be documented in api.rst, please only document find_bad_by_ransac there --> you can copy (or get inspired by) the syntax from the already existing sections.

Lastly, can you please add ransac.find_bad_by_ransac here:

"""initialize pyprep."""
from pyprep.find_noisy_channels import NoisyChannels # noqa: F401
from pyprep.prep_pipeline import PrepPipeline # noqa: F401
from ._version import get_versions
__version__ = get_versions()["version"]
del get_versions

for example:

 import ransac  # noqa: F401 

So that users can do from pyprep import ransac and then call ransac.find_bad_by_ransac

(actually this might be possible also without editing init.py, please feel free to check prior to changing it)

docs/whats_new.rst Outdated Show resolved Hide resolved
docs/whats_new.rst Outdated Show resolved Hide resolved
pyprep/find_noisy_channels.py Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Jan 1, 2021

Codecov Report

Merging #51 (4d915bd) into master (bfb2ab1) will increase coverage by 0.03%.
The diff coverage is 97.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
+ Coverage   96.76%   96.80%   +0.03%     
==========================================
  Files           5        6       +1     
  Lines         526      532       +6     
==========================================
+ Hits          509      515       +6     
  Misses         17       17              
Impacted Files Coverage Δ
pyprep/ransac.py 96.96% <96.96%> (ø)
pyprep/find_noisy_channels.py 96.34% <100.00%> (-0.16%) ⬇️

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 bfb2ab1...4d915bd. Read the comment docs.

@sappelhoff
Copy link
Owner

@yjmantilla I pushed several commits to master to fix the documentation links and improve a few maintenance features.

I now rebased your PR on top of these changes, and also added the new module to the API docs (see 67a4d2c)

before continuing to work you will have to:

git pull
git reset --hard upstream/master
git pull

then it should tell you that everything is up to date and fine. (assuming you have a git remote "upstream" configured to point to sappelhoff/pyprep)

I just hope you didn't have non-committed changes in your working directory :-)

@yjmantilla
Copy link
Collaborator Author

I have not forgotten this yet, just that now I'm doing the final projects of the semester so haven't got time to work on this until around the 25 of January.

Cheers! And Happy New Year!

@sappelhoff
Copy link
Owner

Happy new year to you too @yjmantilla!

@sappelhoff
Copy link
Owner

I finally got the links within the docs working :-)

working beautifully now: https://pyprep.readthedocs.io/en/latest/generated/find_noisy_channels.NoisyChannels.html#find_noisy_channels.NoisyChannels

try to hover over the types like "float" or "np.random.RandomState" --> these are now links that will lead you to the docs of Python3 or Numpy respectively.

I rebased this PR on my changes, you should be able to just continue where you left off once you find time 👍

@yjmantilla yjmantilla requested a review from sappelhoff February 6, 2021 01:02
@yjmantilla
Copy link
Collaborator Author

Sorry for the lateness (rough year start). Everything should be in place now. After this is merged next step is to make the example file comparing autoreject's and pyprep's versions of ransac.

@sappelhoff sappelhoff merged commit 8476ea5 into sappelhoff:master Feb 6, 2021
@sappelhoff
Copy link
Owner

Thanks @yjmantilla hope nothing bad happend

@sappelhoff sappelhoff added this to the 0.3.2 milestone Apr 8, 2021
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.

3 participants