Skip to content

Accept that NaN ≈ NaN in test_approx #220

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 1 commit into
base: main
Choose a base branch
from
Open

Accept that NaN ≈ NaN in test_approx #220

wants to merge 1 commit into from

Conversation

oxinabox
Copy link
Member

@oxinabox oxinabox commented Oct 1, 2021

I think this is the behavior we want.
If you get a actual value of NaN and you were expecting a NaN then all seems well.
If both the AD rule and the finite differencing take you here all is well.
(not ok if only one does)

@codecov-commenter
Copy link

Codecov Report

Merging #220 (447c9e9) into master (683e023) will decrease coverage by 0.33%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #220      +/-   ##
==========================================
- Coverage   90.75%   90.42%   -0.34%     
==========================================
  Files          11       11              
  Lines         303      303              
==========================================
- Hits          275      274       -1     
- Misses         28       29       +1     
Impacted Files Coverage Δ
src/check_result.jl 88.23% <100.00%> (-1.48%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 683e023...447c9e9. Read the comment docs.

@oxinabox
Copy link
Member Author

oxinabox commented Oct 1, 2021

This is not quite the same as JuliaDiff/FiniteDifferences.jl#192
which woulderror if the input is not finite.
Here we are allow output to be NaN,
so we could do both

Copy link
Member

@mzgubic mzgubic left a comment

Choose a reason for hiding this comment

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

needs a version bump and some tests?

@oxinabox
Copy link
Member Author

oxinabox commented Oct 1, 2021

idk if this is a good idea or not

@mzgubic
Copy link
Member

mzgubic commented Oct 1, 2021

The only thing I can think of going wrong is getting a NaN for the wrong reason. But seems relatively unlikely (and harmless in the end, at least at the tested point?)

# 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.

3 participants