Skip to content

Fixed a corner case where numpy's np.float32 nans are not ignored when using ignore_nan_equality #376

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

Merged
merged 1 commit into from
Feb 24, 2023

Conversation

noamgot
Copy link

@noamgot noamgot commented Feb 12, 2023

This PR fixes the following case:

DeepDiff(np.array([1, 2, 3, np.nan], dtype=np.float32), np.array([1, 2, 4, np.nan], dtype=np.float32), ignore_nan_inequality=True)
# Outputs:
{'values_changed': {'root[2]': {'new_value': 4.0, 'old_value': 3.0},
  'root[3]': {'new_value': nan, 'old_value': nan}}}

As you can see, when there are other differences and we are using np.float32 typed arrays, the np.nan values are not ignored, despite setting ignore_nan_inequality=True.

Notice that in case of the default np.float64 type, it doesn't happen:

DeepDiff(np.array([1, 2, 3, np.nan]), np.array([1, 2, 4, np.nan]), ignore_nan_inequality=True)
# Outputs:
{'values_changed': {'root[2]': {'new_value': 4.0, 'old_value': 3.0}}}

Also, no diffs are observed when there are no other differences:

DeepDiff(np.array([1, 2, 3, np.nan], dtype=np.float32), np.array([1, 2, 3, np.nan], dtype=np.float32), ignore_nan_inequality=True)
# Outputs:
{}

So I think I made it clear that this corner case has not been mitigated properly.
This is my first PR, so please tell me if there's anything I can improve...
Thanks!

Copy link
Owner

@seperman seperman left a comment

Choose a reason for hiding this comment

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

LGTM!

@codecov-commenter
Copy link

Codecov Report

Merging #376 (4cb8400) into dev (8ab1c8d) will decrease coverage by 0.19%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##              dev     #376      +/-   ##
==========================================
- Coverage   99.24%   99.06%   -0.19%     
==========================================
  Files          14       14              
  Lines        3161     3192      +31     
==========================================
+ Hits         3137     3162      +25     
- Misses         24       30       +6     
Impacted Files Coverage Δ
deepdiff/diff.py 99.63% <100.00%> (+<0.01%) ⬆️
deepdiff/helper.py 98.73% <100.00%> (+<0.01%) ⬆️
deepdiff/commands.py 95.68% <0.00%> (-4.32%) ⬇️
deepdiff/serialization.py 97.36% <0.00%> (-0.41%) ⬇️
deepdiff/model.py 99.75% <0.00%> (+<0.01%) ⬆️
deepdiff/operator.py 96.42% <0.00%> (+1.19%) ⬆️

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

@seperman seperman merged commit 15a6f80 into seperman:dev Feb 24, 2023
@seperman
Copy link
Owner

I will keep you posted when it is published. Thanks!

@noamgot
Copy link
Author

noamgot commented Feb 25, 2023

Thanks @seperman!

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.

3 participants