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

Closes #3757 ak.array gives unexpected results on a transposed numpy multi dimensional array #3761

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ajpotts
Copy link
Contributor

@ajpotts ajpotts commented Sep 11, 2024

Closes #3757 ak.array gives unexpected results on a transposed numpy multi dimensional array

@ajpotts ajpotts force-pushed the 3757-ak.array-gives-unexpected-results-on-a-transposed-numpy-multi-dimensional-array branch 2 times, most recently from cc3860d to 8780455 Compare September 11, 2024 00:19
@ajpotts ajpotts marked this pull request as ready for review September 11, 2024 00:19
@ajpotts ajpotts force-pushed the 3757-ak.array-gives-unexpected-results-on-a-transposed-numpy-multi-dimensional-array branch 2 times, most recently from fb2cf69 to 8f8a0b8 Compare September 18, 2024 20:52
Comment on lines +294 to +295
# Make a copy to avoid error #3757
a = a.copy()
Copy link
Member

Choose a reason for hiding this comment

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

Do we know why this fixes the error? I haven't thought about it much but if we don't have to create a copy, that's prob better right?

Copy link
Member

@stress-tess stress-tess Sep 18, 2024

Choose a reason for hiding this comment

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

I mean, I guess it's not that a big of a deal bc if it was passed into ak.array was relatively small

Copy link
Member

Choose a reason for hiding this comment

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

yah it's exactly what I thought. when numpy does a transpose, it doesn't actually move any data around, it just takes the same base array in memory and switches it from column_major to row_major or vice versa (I forget which one C used vs Fortran)

>>> nda.flags
  C_CONTIGUOUS : True
  F_CONTIGUOUS : False
  OWNDATA : True
  WRITEABLE : True
  ALIGNED : True
  WRITEBACKIFCOPY : False

>>> npa = np.transpose(nda)

>>> npa.flags
  C_CONTIGUOUS : False
  F_CONTIGUOUS : True
  OWNDATA : False
  WRITEABLE : True
  ALIGNED : True
  WRITEBACKIFCOPY : False

>>> npa.copy().flags
  C_CONTIGUOUS : True
  F_CONTIGUOUS : False
  OWNDATA : True
  WRITEABLE : True
  ALIGNED : True
  WRITEBACKIFCOPY : False

When we do a deep copy, it actually allocates new memory and interprets it correctly. We don't have support for different orders with the current impl. We used to have it with arrayview but that had it's own mess of problems. I guess we could try to check if the OWNDATA flag is false and then deep copy but idk if it's worth it

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 I'll just reference this comment in the issue for future reference and approve for now. I'll leave this unresolved so other reviewers can see it bc I think it kinda interesting. And i'd like to get other opinions

Copy link
Member

@stress-tess stress-tess left a comment

Choose a reason for hiding this comment

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

nice work!! this must've been a pain to track down

@ajpotts ajpotts force-pushed the 3757-ak.array-gives-unexpected-results-on-a-transposed-numpy-multi-dimensional-array branch 2 times, most recently from 877bfb5 to d66bcd1 Compare September 19, 2024 13:27
@ajpotts ajpotts force-pushed the 3757-ak.array-gives-unexpected-results-on-a-transposed-numpy-multi-dimensional-array branch from d66bcd1 to ebe56dd Compare September 20, 2024 16:23
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.

ak.array gives unexpected results on a transposed numpy multi-dimensional array.
2 participants