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

added zero_tol parameter for check_partials method #3166

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kikutabryan
Copy link

Summary

  • Added parameter to check_partials method called zero_tol.
  • If calculated magnitude of partials is below zero_tol, they are set equal to zero to avoid situations where the check magnitude is zero and the calculated magnitude has some slight error resulting in a relative error of 1.

Before:

image

After:

image

Related Issues

Backwards incompatibilities

None

New Dependencies

None

@kikutabryan kikutabryan marked this pull request as draft April 1, 2024 14:22
@kikutabryan kikutabryan marked this pull request as ready for review April 1, 2024 15:54
@coveralls
Copy link

coveralls commented Apr 1, 2024

Coverage Status

coverage: 89.699% (-0.3%) from 89.949%
when pulling 4d615cf on kikutabryan:master
into b46639a on OpenMDAO:master.

@robfalck
Copy link
Contributor

Thanks for your contribution! Would you be able to add a test for our CI that verifies that this works?

It can be as simple as capturing the text from your example above and making sure it matches the expected value.
If you dont think you have the time to do this let us know.

@robfalck
Copy link
Contributor

robfalck commented May 2, 2024

After testing this implementation a bit, I have a few suggestions.

I think zero_tol should specify the magnitude of the calc_mag below which the relative error is not calculated.
A tol on the derivatives themselves overlaps with abs_err_tol and rel_err_tol and is a bit confusing.

Interested to hear thoughts on this.

@kikutabryan
Copy link
Author

After testing this implementation a bit, I have a few suggestions.

I think zero_tol should specify the magnitude of the calc_mag below which the relative error is not calculated. A tol on the derivatives themselves overlaps with abs_err_tol and rel_err_tol and is a bit confusing.

Interested to hear thoughts on this.

Not entirely sure if I understood correctly, but I just updated the to code use the norm of the Jacobian and if the norm is greater than the specified threshold (renamed zero_tol to rel_err_norm_thresh), the relative error will be computed, if it is below, it will not be computed (as you suggested).

Let me know if this sounds better.

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.

Add Zero Tolerance to Check Partials for Relative Error Checks
4 participants