Skip to content

Loosen tolerance on test #495

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Loosen tolerance on test #495

wants to merge 4 commits into from

Conversation

willtebbutt
Copy link
Member

Summary
Replaces an equality test with an approximate equality test to handle roundoff issues. Should resolve #494

Proposed changes
Loosen a test tolerance very slightly

What alternatives have you considered?
None

Breaking changes
None

@codecov
Copy link

codecov bot commented Feb 27, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (8746034) 94.25% compared to head (9e30e87) 94.25%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #495   +/-   ##
=======================================
  Coverage   94.25%   94.25%           
=======================================
  Files          52       52           
  Lines        1374     1374           
=======================================
  Hits         1295     1295           
  Misses         79       79           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@willtebbutt
Copy link
Member Author

@mjp98 if you could confirm that this seems to fix things on your end, I'll merge

Comment on lines +18 to +20
@test isapprox(
kernelmatrix(k, x, y), kernelmatrix(k, collect(x), collect(y)); rtol=1e-6
)
Copy link
Member

Choose a reason for hiding this comment

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

Is the non-standard rtol needed, in particular if we fix the seeds with StableRNGs? Maybe

Suggested change
@test isapprox(
kernelmatrix(k, x, y), kernelmatrix(k, collect(x), collect(y)); rtol=1e-6
)
@test kernelmatrix(k, x, y) kernelmatrix(k, collect(x), collect(y))

would be sufficient?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Independent mokernel failing numerical equality test randomly
3 participants