Skip to content

Verify pipeline patch#30

Open
PranY wants to merge 3 commits into
fastaudio:masterfrom
PranY:verify_pipeline
Open

Verify pipeline patch#30
PranY wants to merge 3 commits into
fastaudio:masterfrom
PranY:verify_pipeline

Conversation

@PranY

@PranY PranY commented Apr 3, 2020

Copy link
Copy Markdown
Contributor

The verify_pipeline method covers the known edge-cases for the transforms pipeline. The design is to introduce an extensible framework to check future cases.

@review-notebook-app

Copy link
Copy Markdown

Check out this pull request on  ReviewNB

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.

Comment thread nbs/01_augment.ipynb
@@ -49,6 +49,9 @@
"outputs": [],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The verify_pipeline function defined here should be instead on the Pipelines section, that is a couple of cells below. Also, it would be very helpful to include some examples, and you can add tests that it works using test_warns function from fastcore.


Reply via ReviewNB

@scart97

scart97 commented Apr 11, 2020

Copy link
Copy Markdown
Collaborator

The tests are breaking because of a problem while exporting the code from the notebooks. Please follow the instructions to fix it:

  • Install nbdev: pip install nbdev --upgrade
  • Install the git hooks (only need to do once): nbdev_install_git_hooks
  • Before each commit run the following commands:
pip install nbdev --upgrade
nbdev_clean_nbs
nbdev_build_lib
nbdev_build_docs
  • Optionally run nbdev_test_nbs to test the code

The instructions to contribute that are present on the repository at the moment are lacking, I will update them.

Comment thread README.md Outdated
@mogwai

mogwai commented Apr 14, 2020

Copy link
Copy Markdown
Member

The instructions to contribute that are present on the repository at the moment are lacking, I will update them.

Surely we ca add those the into the pre-commit hook?

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