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: don't touch for ascontiguousarray #2397

Merged
merged 5 commits into from
Apr 13, 2023

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Apr 13, 2023

This is because touching should really only occur if the operation result depends upon the array value, which it does not in this instance.

This PR

  1. allows dask-awkward to set pretend_contiguous on the ndarray.dtype for our "fake" buffers, ensuring that nplikes do not force them to become contiguous.
  2. does not touch arrays in ascontiguousarray

The plan is to remove (1) later, once dask-awkward passes in its own special content types.

This is because touching should really only occur if the operation result _depends_ upon the array value, which it does not in this instance.
@agoose77 agoose77 requested a review from jpivarski April 13, 2023 14:17
@agoose77 agoose77 marked this pull request as ready for review April 13, 2023 14:17
@agoose77 agoose77 temporarily deployed to docs-preview April 13, 2023 14:31 — with GitHub Actions Inactive
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

We talked about this in our meeting, and the key point is

The plan is to remove (1) later, once dask-awkward passes in its own special content types.

I'm not comfortable using an under-documented NumPy feature (np.dtype.metadata), particularly since pretend_contiguous is a statement about its shape and strides (everything but the dtype). However, the dtype follows the array, so it's expedient to do it this way for now.

This will go into the next release.

@jpivarski jpivarski enabled auto-merge (squash) April 13, 2023 16:27
@lgray
Copy link
Contributor

lgray commented Apr 13, 2023

What do we need to do over in dask awkward to make it function as expected?

I'm not sure if we've actually hit this one yet, given that the other PR fixed the issue I was running into with Yi-Mu.

@jpivarski jpivarski temporarily deployed to docs-preview April 13, 2023 16:47 — with GitHub Actions Inactive
@jpivarski
Copy link
Member

We'll be able to avoid relying on np.dtype.metadata by completing dask-contrib/dask-awkward#203 with a Dummy version of NumpyArray ("Dummy" → "Placeholder"). By rehydrating arrays on Dask workers with Dummy subclasses of Index and NumpyArray, we'll be able to make operations like ascontiguousarray be no-ops for the rehydrated array, just as they are (with this PR) for the TypeTracerArray.

The current method of rehydrating with zero-strided (but otherwise real NumPy) arrays doesn't do that.

What I'm addressing here is a preferred refactoring; I don't know if you were asking about something more fundamental, like why this PR exists at all.

@lgray
Copy link
Contributor

lgray commented Apr 13, 2023

Oh I was trying to understand what additional PRs would be needed in dask-awkward before minting another release, and it sounds like None until we run into this problem there.

@codecov
Copy link

codecov bot commented Apr 13, 2023

Codecov Report

Merging #2397 (764c052) into main (60790fa) will decrease coverage by 0.02%.
The diff coverage is 56.66%.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/_nplikes/cupy.py 42.46% <0.00%> (ø)
src/awkward/_kernels.py 66.66% <41.17%> (-2.27%) ⬇️
src/awkward/_nplikes/jax.py 88.88% <50.00%> (+4.27%) ⬆️
src/awkward/_nplikes/array_module.py 90.57% <75.00%> (-0.60%) ⬇️
src/awkward/_nplikes/numpy.py 64.00% <100.00%> (ø)
src/awkward/_nplikes/numpylike.py 73.81% <100.00%> (ø)
src/awkward/_nplikes/typetracer.py 77.29% <100.00%> (-0.04%) ⬇️
src/awkward/contents/numpyarray.py 91.41% <100.00%> (-0.05%) ⬇️

@jpivarski jpivarski merged commit 2ef498e into main Apr 13, 2023
@jpivarski jpivarski deleted the agoose77/fix-ascontiguousarray-touching branch April 13, 2023 17:19
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