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

Docs/Improve documentation #65

Merged
merged 1 commit into from Nov 18, 2021

Conversation

yarkhinephyo
Copy link
Contributor

@yarkhinephyo yarkhinephyo commented Nov 16, 2021

Summary

Issue #28

  • Fix spellings
  • Fix docstrings
    • Example: Missing callable in columns : str or iterable, optional
  • Add code examples when use cases are complex
    • Trying to use existing tests when possible
    • If cannot, tests are added
  • Enable links for crucial references
    • Example: pdpipe.cq to pdpipe.cq

Progress

  • documentation.md
  • pdpipe.basic_stages
  • pdpipe.col_generation
  • pdpipe.cond
  • pdpipe.core
  • pdpipe.cq
  • pdpipe.nltk_stages
  • pdpipe.skintegrate
  • pdpipe.sklearn_stages
  • pdpipe.text_stages
  • pdpipe.wrappers

@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #65 (957276b) into master (f14581d) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 957276b differs from pull request most recent head e57988e. Consider uploading reports for the commit e57988e to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##            master       #65   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        14           
  Lines         2033      2033           
  Branches       271       271           
=========================================
  Hits          2033      2033           
Impacted Files Coverage Δ
pdpipe/col_generation.py 100.00% <ø> (ø)
pdpipe/core.py 100.00% <ø> (ø)
pdpipe/cq.py 100.00% <ø> (ø)
pdpipe/skintegrate.py 100.00% <ø> (ø)
pdpipe/text_stages.py 100.00% <ø> (ø)
pdpipe/wrappers.py 100.00% <ø> (ø)
pdpipe/basic_stages.py 100.00% <100.00%> (ø)
pdpipe/cond.py 100.00% <100.00%> (ø)
pdpipe/nltk_stages.py 100.00% <100.00%> (ø)
pdpipe/sklearn_stages.py 100.00% <100.00%> (ø)
... and 2 more

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 f14581d...e57988e. Read the comment docs.

@yarkhinephyo
Copy link
Contributor Author

Hi @shaypal5

  • pdpipe.util.per_column_valus_sklearn_transform function seems to have spelling mistake. Should I make an issue?
  • exclude_columns in ColumnsBasedPipelineStage does not work with pdpipe.cq even though the documentation mentions consistently that it works. There are no relevant tests for cq + exclude_columns too so it should not be implemented yet. Is it a work in progress or is there a mistake in the documentation?

pdpipe/basic_stages.py Outdated Show resolved Hide resolved
pdpipe/col_generation.py Outdated Show resolved Hide resolved
@shaypal5
Copy link
Collaborator

Looks very good so far! Great work. :)

Regarding your points:

  1. Just fix them :)
  2. Yeah, I know about this and I'll get to this soon (see issue Have column-based stages successfully support column qualifiers as arguments #58). You can add tests that should work and mark them with @pytest.mark.xfail - I already have one testing this and failing.

@shaypal5
Copy link
Collaborator

Let me know when you're finished, and remember - you must rebase all commits to a single one when you're done and want me to merge this.

@shaypal5 shaypal5 mentioned this pull request Nov 16, 2021
@yarkhinephyo
Copy link
Contributor Author

Oh there are two spelling issues in main page. I cannot locate in the repository.

image

@shaypal5
Copy link
Collaborator

Nice catch. I'll get them. They're on a different branch.
Let me know when you're done. :)

@yarkhinephyo yarkhinephyo changed the title [WIP] Docs/Improve documentation Docs/Improve documentation Nov 18, 2021
@yarkhinephyo
Copy link
Contributor Author

@shaypal5 The PR is ready and I have squashed the commits into a single commit :)

Improve basic_stages docs

Fix minor mistake in basic_stages

Rename label to single label

Improve col_generation docs

Improve cond docs

Improve core docs

Improve cq docs

Undo minor docstring change in col-generation

Fix pr comments

Improve text_stages docs

Improve wrappers docs

Improve sklearn_stages doc

Improve skintegrate docs

Improve nltk_stages documentation

Improve documentation.md by adding links
Copy link
Collaborator

@shaypal5 shaypal5 left a comment

Choose a reason for hiding this comment

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

Amazing work! So meticulous. Thank you for such a great contribution. :)

@shaypal5 shaypal5 merged commit 238c46f into pdpipe:master Nov 18, 2021
@shaypal5
Copy link
Collaborator

A small request: Next time, please run flake8 before submitting the PR. You had 48 flake8 errors. You should probably have your IDE (whatever you use) configured so it output flake8-compliant files.

You had missing newlines at the end of a file, and a lot of trailing whitespaces at the end of lines. These are things that should automatically be handled by any proper IDE.

@yarkhinephyo yarkhinephyo deleted the docs/improve-documentation branch November 18, 2021 11:08
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.

None yet

2 participants