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

Do not require explicit RNG in verify_grad #1093

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

t3chw
Copy link
Contributor

@t3chw t3chw commented Nov 17, 2024

Ensures that verify_grad can handle cases where an RNG is not explicitly provided

Description

Ensures that verify_grad can handle cases where an RNG is not explicitly provided, improving the function's usability and robustness. Added a new test case, test_verify_grad_no_rng, to verify that the verify_grad function works correctly without requiring an explicit RNG (random number generator) argument.

Related Issue

Checklist

Type of change

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

@@ -602,6 +603,10 @@ def test_grad_constant(self):
+ str(g_one)
)

def test_verify_grad_no_rng(self):
Copy link
Member

Choose a reason for hiding this comment

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

No need for the test, the changes in the code are okay by themselves

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve removed the test as suggested. Let me know if there’s anything else to address!

Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

Thanks

Copy link

codecov bot commented Nov 18, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 82.12%. Comparing base (33a4d48) to head (00c99f3).
Report is 92 commits behind head on main.

Files with missing lines Patch % Lines
pytensor/gradient.py 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1093      +/-   ##
==========================================
+ Coverage   82.11%   82.12%   +0.01%     
==========================================
  Files         183      183              
  Lines       47959    47978      +19     
  Branches     8635     8642       +7     
==========================================
+ Hits        39381    39403      +22     
+ Misses       6411     6408       -3     
  Partials     2167     2167              
Files with missing lines Coverage Δ
pytensor/gradient.py 77.57% <0.00%> (ø)

... and 3 files with indirect coverage changes

@t3chw
Copy link
Contributor Author

t3chw commented Nov 18, 2024

The pull request is ready for merge:

  • Approved by the reviewer.
  • No conflicts with the main branch.
  • The only failing check is codecov/patch due to 0% coverage on the diff, which is expected after removing the suggested test.

Let me know if there’s anything else I should address!

@ricardoV94 ricardoV94 merged commit 0ba554b into pymc-devs:main Nov 18, 2024
61 of 62 checks passed
@ricardoV94
Copy link
Member

The pull request is ready for merge:

  • Approved by the reviewer.
  • No conflicts with the main branch.
  • The only failing check is codecov/patch due to 0% coverage on the diff, which is expected after removing the suggested test.

Let me know if there’s anything else I should address!

Thanks again. Hope you don't mind me asking, is this a robot message?

@t3chw
Copy link
Contributor Author

t3chw commented Nov 18, 2024

Haha, no not a robot, just filtered my thought with chat-gpt. This is my first PR in pymc-dev, will try to contribute more and continuously learning about pymc.

@ricardoV94 ricardoV94 changed the title Add test for 'verify_grad' without explicit RNG Do not require explicit RNG in 'verify_grad' Nov 18, 2024
@ricardoV94 ricardoV94 changed the title Do not require explicit RNG in 'verify_grad' Do not require explicit RNG in verify_grad Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not require rng in verify_grad
2 participants