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

fix issue where roundtripping a masked array strips the mask if no values are masked #1803

Merged

Conversation

zacharyburnett
Copy link
Member

@zacharyburnett zacharyburnett commented Jul 17, 2024

Description

resolves #1768

Checklist:

  • pre-commit checks ran successfully
  • tests ran successfully
  • for a public change, a changelog entry was added
  • for a public change, documentation was updated
  • for any new features, unit tests were added

@zacharyburnett zacharyburnett self-assigned this Jul 17, 2024
@zacharyburnett zacharyburnett marked this pull request as ready for review July 17, 2024 18:49
@zacharyburnett zacharyburnett requested a review from a team as a code owner July 17, 2024 18:49
@braingram
Copy link
Contributor

Would you add a changelog for this that documents the fixed bug?

Also, would you either move the test near:
https://github.com/asdf-format/asdf/blob/main/asdf/_tests/tags/core/tests/test_ndarray.py#L381
or update that test to the nicely parametrized one added in this PR?

@zacharyburnett zacharyburnett changed the title handle case where masked array has no true fix issue where roundtripping a masked array where no values are masked strips the mask Jul 18, 2024
@zacharyburnett zacharyburnett force-pushed the fix/false_masked_array_roundtrip branch from 92c98da to d693b19 Compare July 18, 2024 13:13
@braingram
Copy link
Contributor

There's one more issue and I'm a little scared now (not from this PR but from what follows):

import numpy as np
arr = np.arange(5)
m0 = np.ma.masked_array(arr, True)
m1 = np.ma.masked_array(arr, False)
np.testing.assert_array_equal(m0, m11)

The last assert does not fail :-/ so apparently assert_array_equal does not consider the mask. The same is true for assert_equal. Additionally (and this part is asdf's fault), assert_tree_match adds an extra oddity:

np.testing.assert_array_equal(old.__array__(), new.__array__())

which drops the mask.

Would you update the test to not use assert_tree_match and apparently (maybe any) of the numpy testing routines. I'll open an issue to check our test suite for other places where the mask is being ignored.

@zacharyburnett zacharyburnett force-pushed the fix/false_masked_array_roundtrip branch from 00c9703 to 503595c Compare July 18, 2024 13:50
@zacharyburnett zacharyburnett changed the title fix issue where roundtripping a masked array where no values are masked strips the mask fix issue where roundtripping a masked array strips the mask if no values are masked Jul 18, 2024
Co-authored-by: Brett Graham <brettgraham@gmail.com>
@braingram
Copy link
Contributor

To add to the confusion, the test is failing because:

>> (m0 == m1).all()
masked

when m0 and m1 are both fully masked and matching values. So I think we need to compare m0.mask and m0.data separately.

Copy link
Contributor

@braingram braingram left a comment

Choose a reason for hiding this comment

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

Thanks! One more bug squished.

@braingram braingram merged commit b8ba0ad into asdf-format:main Jul 18, 2024
34 checks passed
@zacharyburnett zacharyburnett deleted the fix/false_masked_array_roundtrip branch July 19, 2024 12:02
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.

masked arrays do not roundtrip with all false masks
2 participants