-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add JVP/VJP type checking in Catalyst frontend #1031
Conversation
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.
Good job! I would just split the single test into multiple ones
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1031 +/- ##
=======================================
Coverage 97.88% 97.88%
=======================================
Files 75 75
Lines 10685 10702 +17
Branches 1226 1235 +9
=======================================
+ Hits 10459 10476 +17
Misses 177 177
Partials 49 49 ☔ View full report in Codecov by Sentry. |
@joeycarter Also for the the code factor issue, you can apply |
3a1be58
to
739dd4a
Compare
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.
Thanks, great job 💯
Context: Improve type checking and error messaging in the Catalyst JVP and VJP functions.
Description of the Change: Adds type checking to ensure the following for JVP:
and for VJP:
Note that the equivalent type checking is also performed at the MLIR level.
Benefits: Checking functional parameters earlier in the frontend improves error messaging and usability rather than falling back to the MLIR error messages.
Possible Drawbacks: There's a small risk that stricter type checking in the frontend might break backward compatibility if users are calling these functions in unconventional or non-standard ways that are not captured by our unit/integration tests.
Related GitHub Issues: