Skip to content

Grad error message #721

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

Closed
wants to merge 17 commits into from
Closed

Grad error message #721

wants to merge 17 commits into from

Conversation

tanish1729
Copy link
Contributor

@tanish1729 tanish1729 commented Apr 17, 2024

Description

Created a new branch and added grad_not_implemented for a helpful error message

Related Issue

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

@tanish1729
Copy link
Contributor Author

@ricardoV94 I still seem to getting all my old commits on the new branch I used. Since this is just a one-line fix for the issue, can we just go through the changes here and finish this. I'll make a new PR for a different issue after this and make sure these mistakes dont happen

@@ -357,7 +358,10 @@ def grad(
.. [1] Giles, Mike. 2008. “An Extended Collection of Matrix Derivative Results for Forward and Reverse Mode Automatic Differentiation.”

"""
raise NotImplementedError()
# raise NotImplementedError()
Copy link
Member

Choose a reason for hiding this comment

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

This is my suggestion for a simpler solution

Suggested change
# raise NotImplementedError()
raise NotImplementedError(f"grad not implemented for Op {self}")

Copy link
Member

Choose a reason for hiding this comment

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

Also your implementation was wrong because it assumed there is a single input

@tanish1729 tanish1729 closed this by deleting the head repository Jun 2, 2024
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