Skip to content

Conversation

@spott
Copy link

@spott spott commented Jul 1, 2025

This just changes the np.array_equal to a new version that works for ndarrays of ndarrays (object dtype) in set_attribute_all.

All tests pass, though this should probably have a new test to prevent regressions.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a fix for deep comparison of numpy ndarrays with object dtype in the set_attributes_all function. The standard np.array_equal function doesn't handle nested ndarrays (object arrays containing ndarrays) properly, so this change implements a custom deep comparison function.

Key changes:

  • Added new deep_array_equal function for robust comparison of object arrays containing ndarrays
  • Updated set_attributes_all to use the new deep comparison function instead of np.array_equal

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

This is a more robust version of np.array_equal that works for object arrays
that contain ndarrays.
"""
if isinstance(a, np.ndarray) and isinstance(b, np.ndarray) and (a.dtype == object or b.dtype == object):
Copy link

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

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

The condition should check that both arrays have object dtype, not just one of them. Using or could lead to incorrect comparisons when one array is object dtype and the other isn't. Consider changing to a.dtype == object and b.dtype == object

Suggested change
if isinstance(a, np.ndarray) and isinstance(b, np.ndarray) and (a.dtype == object or b.dtype == object):
if isinstance(a, np.ndarray) and isinstance(b, np.ndarray) and (a.dtype == object and b.dtype == object):

Copilot uses AI. Check for mistakes.
Copy link
Author

Choose a reason for hiding this comment

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

I don't believe we want this, as comparisons can still be valid even if only one is an object.

Copy link
Owner

Choose a reason for hiding this comment

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

I think this shouldn't matter either way, since if one is an object and the other isn't then the existing np.array_equal should do the right thing and return false.

@jclds139 jclds139 self-assigned this Aug 14, 2025
@jclds139
Copy link
Owner

So, there's a couple of things:

  • This definitely need some new test cases to make sure we're not about to potentially break something or end up in a recursive loop
  • This seems more like a Numpy issue so I'm wondering if this should be a wontfix and just file an upstream bug. @joshayers do you have an opinion on that?
  • Most crucially, is there a good use case for needing this? As far as I can think of ndarrays shouldn't need to be nested because the whole point of them is to have whatever number of dimensions. In the odd case where you have ndarrays of different sizes that you want to store together I would think that using native python lists makes more sense since you can't exactly do math on nested arrays like that anyway. Feel free to correct me if I'm overlooking something on this point, though.

jclds139

This comment was marked as duplicate.

Copy link
Owner

@jclds139 jclds139 left a comment

Choose a reason for hiding this comment

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

Would need test cases and probably some kind of safeguard to prevent recursion loops, otherwise ready. See previous comment on whether or not we should just kick this upstream.

@jclds139 jclds139 dismissed their stale review August 14, 2025 17:59

Need to decide whether to keep this PR or not before worrying about updating.

@jclds139 jclds139 linked an issue Aug 14, 2025 that may be closed by this pull request
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.

set_attributes_all has an issue with comparing ndarrays of ndarrays.

2 participants