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

Top k value fix palm #609

Merged
merged 2 commits into from
Oct 3, 2023
Merged

Top k value fix palm #609

merged 2 commits into from
Oct 3, 2023

Conversation

Tanmaypatil123
Copy link
Contributor

@Tanmaypatil123 Tanmaypatil123 commented Oct 3, 2023

  • closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.

Bug fix for older top_k value for google palm model.

Fixes :

  1. Changed range of top_k value to [0.0 to 100.0]
  2. Changed default value of top_k value to 40 .
  3. Changed datatype of top_k value from float to integer.

Summary by CodeRabbit

Release Notes for Pull Request:

  • Refactor: Updated the top_k attribute in the BaseGoogle class from float to int, with a new default value of 40. This change will improve the precision of results.
  • Test: Adjusted the test_params_setting and test_validations methods in the test suite to align with the updated top_k attribute.
  • Chore: Added a simple print statement in test_file.txt for debugging purposes.

Please note that these changes are internal and should not affect end-user functionality.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 3, 2023

Walkthrough

The changes primarily revolve around the modification of the top_k attribute in the BaseGoogle class and its corresponding tests. The type of top_k has been altered from float to int, with an updated default value and range validation. A minor addition of a print statement is also included.

Changes

File Path Summary
pandasai/llm/base.py The top_k attribute's type has been changed from float to int, with a new default value of 40. Its validation now ensures it falls within [0, 100].
test_file.txt A line printing "Hello, world!" to the console has been added.
tests/llms/test_google_palm.py The test_params_setting and test_validations methods have been updated to reflect the changes made to the top_k attribute in the BaseGoogle class.

🐇💻

In the land of code where the shadows lie,

An integer replaces a float, oh my!

Tests are adjusted, they do comply,

And a greeting to the world, we slyly apply.

Celebrate these changes, under the sky,

For in the realm of software, we aim high! 🎉


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • Mention @coderabbitai in any review comment for bot assistance.
  • Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

  • Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.
  • Additionally, enter @coderabbitai pause to halt the bot's ongoing reviews as any issue or review comment. To
    resume reviews, enter @coderabbitai resume as any issue or review comment.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Commits Files that changed from the base of the PR and between 42b8256 and 163dab1.
Files selected for processing (3)
  • pandasai/llm/base.py (2 hunks)
  • test_file.txt (1 hunks)
  • tests/llms/test_google_palm.py (2 hunks)
Files skipped from review due to trivial changes (1)
  • test_file.txt
Additional comments (Suppressed): 3
tests/llms/test_google_palm.py (2)
  • 37-48: The top_k parameter in the test_params_setting method has been updated from 0.5 to 50. This change aligns with the PR summary, which states that the type of top_k has been changed from float to int, and its default value is now set to 40. The test case should pass as long as the GooglePalm class correctly handles the new top_k value.

  • 69-79: The range validation for top_k in the test_validations method has been modified from [0.0, 1.0] to [0.0, 100.0]. The negative value check for top_k is updated from -1 to -100, and the upper limit check is changed from 1.1 to 110. These changes are consistent with the PR summary and should ensure that the GooglePalm class correctly validates the top_k parameter.

pandasai/llm/base.py (1)
  • 375-379: The type of top_k has been changed from float to int, and its default value is now set to 40. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify if the change in the default value of top_k does not affect the existing functionality or performance.

Comment on lines +412 to +413
if self.top_k is not None and not 0 <= self.top_k <= 100:
raise ValueError("top_k must be in the range [0.0, 100.0]")
Copy link
Contributor

Choose a reason for hiding this comment

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

The validation range for top_k has been updated from [0.0, 1.0] to [0.0, 100.0]. This change aligns with the update in the type and default value of top_k. However, the error message still mentions the range as [0.0, 100.0] which might be confusing since top_k is now an integer. Consider updating the error message to reflect the correct range as [0, 100].

- raise ValueError("top_k must be in the range [0.0, 100.0]")
+ raise ValueError("top_k must be in the range [0, 100]")

@gventuri gventuri merged commit b31dda7 into Sinaptik-AI:main Oct 3, 2023
9 checks passed
@@ -0,0 +1 @@
Hello, world!
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file supposed to be here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mspronesti it's probably a easter egg from @Tanmaypatil123 😅

Removed it

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