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

HAP transform: Update README.md and add sample notebook #821

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

ian-cho
Copy link
Collaborator

@ian-cho ian-cho commented Nov 22, 2024

update readme for hap transform

Related issue number (if any).

#753

update readme for hap transform
@shahrokhDaijavad
Copy link
Member

Thank you, @ian-cho. The README looks good. Three requests:

  1. Can you please add a "Contributors" section with your name and email at the top like this: https://github.com/IBM/data-prep-kit/tree/update-doc_quality-readme/transforms/language/doc_quality/python?
  2. The link to the test_hap.py file seems to be broken.
  3. We have now finalized the format of the example notebook. Can you please create one like this: https://github.com/IBM/data-prep-kit/blob/update-doc_quality-readme/transforms/language/doc_quality/doc_quality.ipynb? Of course, the content will be similar to what you have in hap_local_python.py and some text from your README about the parameters. Thank you.

Copy link
Member

@shahrokhDaijavad shahrokhDaijavad left a comment

Choose a reason for hiding this comment

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

Please see my comments.

@touma-I touma-I changed the title Update README.md HAP transform: Update README.md and add sample notebook Nov 24, 2024
@shahrokhDaijavad
Copy link
Member

Sorry, @ian-cho. After I gave you the links to the branch above, the branch was merged, so the new links are in the dev branch at:
https://github.com/IBM/data-prep-kit/blob/dev/transforms/language/doc_quality/python/README.md
and
https://github.com/IBM/data-prep-kit/blob/dev/transforms/language/doc_quality/doc_quality.ipynb

1. added contributor
2. added hap notebook
@ian-cho
Copy link
Collaborator Author

ian-cho commented Dec 2, 2024

@shahrokhDaijavad Hi, I updated the PR as requested. Please review, thanks.

  1. added contributor
  2. For the link, replace it with notebook
  3. added the hap python notebook.

@@ -48,6 +56,20 @@ python hap_local_python.py

You will obtain the output file `test1.parquet` in the output directory.

### Code example
[notebook](../hap_python.ipynb)
Copy link
Member

Choose a reason for hiding this comment

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

The link to the notebook is broken. The correct link is: ./hap_python.ipynb, because the notebook is in the python directory.

Copy link
Member

@shahrokhDaijavad shahrokhDaijavad left a comment

Choose a reason for hiding this comment

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

Thank you, @ian-cho, for developing the minimal notebook that is functionally correct. For the consistency of your notebook with others, before the current cell one that imports the necessary packages, can you please add what we add in other notebooks, similar to what is here: https://github.com/IBM/data-prep-kit/blob/dev/transforms/language/doc_quality/doc_quality.ipynb?

@ian-cho
Copy link
Collaborator Author

ian-cho commented Dec 3, 2024

@shahrokhDaijavad Thank you for further suggestions. Before code cells, I added markdown content and configurations for the hap transforms just as doc_quality.ipynb did. Please review.

@shahrokhDaijavad
Copy link
Member

Thank you, @ian-cho. The notebook looks good. And what about the broken link in the README above?

@ian-cho
Copy link
Collaborator Author

ian-cho commented Dec 3, 2024

Thank you, @ian-cho. The notebook looks good. And what about the broken link in the README above?

@shahrokhDaijavad Sorry, I missed that. Fixed it

Copy link
Member

@shahrokhDaijavad shahrokhDaijavad left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you, @ian-cho.

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.

2 participants