-
Notifications
You must be signed in to change notification settings - Fork 19.7k
Fix discretization discrepancy #21769
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
Fix discretization discrepancy #21769
Conversation
Summary of ChangesHello @danielenricocahall, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a minor bug in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request aims to fix a dtype discrepancy in the Discretization layer. While the proposed change of removing the dtype coercion logic does resolve the inconsistency, it introduces a new issue where output_mode="int" produces float outputs, which is counter-intuitive. My review provides a suggestion to fix the original discrepancy while preserving the expected integer output type. I've also recommended strengthening the new test case to include dtype checks for better validation.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #21769 +/- ##
==========================================
- Coverage 82.69% 82.65% -0.04%
==========================================
Files 573 577 +4
Lines 58888 59181 +293
Branches 9218 9273 +55
==========================================
+ Hits 48696 48918 +222
- Misses 7845 7887 +42
- Partials 2347 2376 +29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively resolves a dtype discrepancy in the Discretization layer, particularly when output_mode is set to "int". The changes correctly introduce an output_dtype property to centralize dtype logic, ensuring consistent behavior between symbolic and eager execution paths. The removal of the manual dtype handling in __init__ and the subsequent updates in compute_output_spec and call are well-implemented. The addition of test_model_call_vs_predict_consistency is a great way to safeguard against future regressions. I've only found one minor issue in the new test file, which is a duplicated assertion.
fchollet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the PR!
Resolved minor bugs in #21514 to address #21468 to fix the discrepancy in discretization