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

Update HAP README.md #661

Merged
merged 3 commits into from
Oct 3, 2024
Merged

Update HAP README.md #661

merged 3 commits into from
Oct 3, 2024

Conversation

ian-cho
Copy link
Collaborator

@ian-cho ian-cho commented Oct 3, 2024

updated hap README @shahrokhDaijavad

Why are these changes needed?

  • addressed the issues in the comments about having default values for the configuration for the key values
  • added default values
  • used contents as column name instead of doc_text
  • added a hyperlink for NLTK

Related issue number (if any).

#659

updated hap README
@touma-I
Copy link
Collaborator

touma-I commented Oct 3, 2024

thank @ian-cho for the quick action. Should you also update the code so the default value is also reflected in the column name when the users do NOT pass it as a parameter value? let me know if this makes sense. thanks

More specifically: lines 30, 31 and 32 of hap_transform.py

        self.model_name_or_path = config.get("model_name_or_path")
        self.annotation_column = config.get("annotation_column")
        self.doc_text_column = config.get("doc_text_column")

@touma-I
Copy link
Collaborator

touma-I commented Oct 3, 2024

@ian-cho Please take a look here #638 and see if any of the comments from the previous PR can be addressed in this one. Thanks again. cc: @shahrokhDaijavad

@ian-cho
Copy link
Collaborator Author

ian-cho commented Oct 3, 2024

thank @ian-cho for the quick action. Should you also update the code so the default value is also reflected in the column name when the users do NOT pass it as a parameter value? let me know if this makes sense. thanks

More specifically: lines 30, 31 and 32 of hap_transform.py

        self.model_name_or_path = config.get("model_name_or_path")
        self.annotation_column = config.get("annotation_column")
        self.doc_text_column = config.get("doc_text_column")

Thanks. I am also going to do that. Just wonder if self.model_name_or_path = config.get("model_name_or_path", "ibm-granite/granite-guardian-hap-38m") is okay?

@ian-cho
Copy link
Collaborator Author

ian-cho commented Oct 3, 2024

thank @ian-cho for the quick action. Should you also update the code so the default value is also reflected in the column name when the users do NOT pass it as a parameter value? let me know if this makes sense. thanks

More specifically: lines 30, 31 and 32 of hap_transform.py

        self.model_name_or_path = config.get("model_name_or_path")
        self.annotation_column = config.get("annotation_column")
        self.doc_text_column = config.get("doc_text_column")

made according changes in this pull request #663

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 . LGTM. I will do a new PR adding hap to the table in the root README file.

Added HAP to the table in README
@touma-I touma-I merged commit f0f46de into dev Oct 3, 2024
1 check passed
@touma-I touma-I deleted the ian-cho-patch-1 branch October 3, 2024 18:34
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