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

Possible Solutions to Issues 26216, 25677, and 25589 #26276

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

20revsined
Copy link

BUG: Bug fix to issue 26216 in numpy.lib._function_base_impl.py
Calculate the median date by sorting the list of np.datetime64 values and selecting either the middle value if the list has an odd length or the middle two values if the list has an even length.

If the list is odd, returns the middle date, and if the list is even, it calculates the average distance between the middle two dates, adding this average distance to the earlier middle date.

BUG: Bug fix to issue 25667 in numpy.ma.core.py
Added different custom fill values for different types of integers and floats.

Using str(dtype) acts as the key in the default_filler dictionary to retrieve the custom value. If this key does not exist in default_filler, then dtype.kind is used as the key instead.

BUG: Bug fix to issue 25589 in numpy.ma.core.py
Added shrink = False in the function call to mask_or() in the power() function to preserve the existing mask.

Copy link
Member

@rkern rkern left a comment

Choose a reason for hiding this comment

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

A couple of quick things before people spend time on full reviews:

  • These 3 issues appear to be unrelated. Please make a separate PR for each. It's hard to review when they are mashed together into one PR.
  • We do not use triple-quoted strings for multiline comments. Please use # comments.
  • When making comments, don't repeat what properly belongs in a git commit message/PR description. If the code needs a comment (most don't), describe what the current code is doing, not what you changed.
  • Go ahead and delete unused code.
  • Please add tests for the changed behavior (or modify the existing ones that now fail because of the changes, if indeed the change was intentional).

if isinstance(a, list):
a = np.array(a)

if a.size > 0 and len(a.shape) > 0 and all(isinstance(i, np.datetime64) for i in a):
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't take into account any of the keepdims, axis, out or overwrite_input arguments.

numpy/ma/core.py Outdated
"int32": 2147483647,
"uint32": 4294967295,
"int64": 9223372036854775807,
"uint64": pow(2, 64) - 1,
Copy link
Member

Choose a reason for hiding this comment

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

I think all of the integer dtypes that can represent 999999 should retain that value. Otherwise, we'd have to change all of those tests that are now failing. Similarly, the floating point dtypes should retain the old value.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, thanks for letting me know. I reverted the int types back as follows:

default_filler = {'b': True, 'c': 1.e20 + 0.0j, "float16": 65504, "float32": 1.e20, "float64": 1.e20, "i": 999999, 'O': '?', 'S': b'N/A', 'V': b'???', 'U': 'N/A' }

However, float16 remains 65504 to avoid the inf value per the original issue (#25677).

Does this make sense?

Copy link
Member

Choose a reason for hiding this comment

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

#25677 is still unresolved for the smaller integer types. Write out the unit tests that test for the behavior that you want. Demonstrate (to yourself) that they fail without your changes, then add your changes until the tests pass.

numpy/ma/core.py Outdated Show resolved Hide resolved
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.

None yet

2 participants