Skip to content

Conversation

dkazanc
Copy link
Collaborator

@dkazanc dkazanc commented Aug 22, 2025

Fixes #619

An additional note: The data input to rescale_to_int (CPU) needs to be C-contiguous and one approach would be to do the following using Numpy's API instead of the CuPy's API as before in here:

   if not gpu_enabled or xp.get_array_module(self.data).__name__ == "numpy":
        self._data = np.asarray(self.data, order="C")
        return

However, I decided to add conversion to C-contiguous on the method side as this is the requirement of the method, mostly due to the C-wrapped code we use. Also when the data on the CPU is converted to C-contiguous by the framework it is counted by the montior as a GPU transfer, which is confusing when plotting the times.

However, it would be interesting to know why the data becomes non C-contiguous when it is written in the sink or read by the source. It is a chance to remove unnecessary data copy with np.asarray(data, order="C")

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have made corresponding changes to the documentation

@yousefmoazzam
Copy link
Collaborator

I'm curious, is the order="C" that's passed to np.empty() making any difference to whatever behaviour was observed?

We use numpy v1.26.x from what I see in the CI, and that version's docs for np.empty say that order has a default value of "C", so naively I would have thought that adding in order="C" doesn't do anything different?

@yousefmoazzam
Copy link
Collaborator

However, it would be interesting to know why the data becomes non C-contiguous when it is written in the sink or read by the source.

If you're talking about the numpy array inside a block that is either:

  • read from a source
  • written to a sink

I would imagine that, in general, those numpy arrays are not C-contiguous.

This is because numpy arrays inside blocks that are read from a source or written to a sink originate from slicing the original numpy array that represents the chunk associated with a process for a given section, and generating a numpy array via slicing another numpy array doesn't produce a C-contiguous numpy array in general.

Only under certain circumstances will slicing produce a C-contiguous numpy array, due to how slicing may or may not produce a view of the original data that is still C-contiguous (in particular, whether the elements within the view are stored contiguously in row-major order or not):

>>> import numpy as np
>>> arr = np.empty((5, 10, 10))
>>> arr.flags.c_contiguous
True
>>> first_dim_slice = arr[1:3, :, :]
>>> first_dim_slice.flags.c_contiguous
True
>>> second_dim_slice = arr[:, 2:4, :]
>>> second_dim_slice.flags.c_contiguous
False

So, my naive assumption would be that numpy arrays representing chunks are C-contiguous (due to being created by np.empty() and copying relevant data into them), but numpy arrays representing blocks in general are not C-contiguous.

As a reference, numpy docs here mention how slicing a numpy array often produces a "view" of the original numpy array.

@dkazanc
Copy link
Collaborator Author

dkazanc commented Sep 1, 2025

Thanks for the clarification @yousefmoazzam . You're, indeed, right about default: ‘C’for the np.empty function and I see what you mean about the non C-contiguous blocks.
As I mentioned, we're converting non C-contiguous on the function side, when it is needed, so essentially no changes needed for the framework. I guess the main change here is to prevent CuPy API not to be applied to numpy arrays.
The last bit is a little change for monitor where do not need to track time for D2H/H2D for CPU methods.

Copy link
Collaborator

@yousefmoazzam yousefmoazzam left a comment

Choose a reason for hiding this comment

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

Apologies, I forgot about this PR!

Ok, it sounds like adding order="C" to the np.empty() calls indeed makes no difference, so could those be removed please.

@dkazanc
Copy link
Collaborator Author

dkazanc commented Sep 12, 2025

OK to merge @yousefmoazzam ?

@dkazanc dkazanc merged commit c7ed01c into main Sep 12, 2025
6 checks passed
@dkazanc dkazanc deleted the block_to_cpu branch September 12, 2025 09: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.

CuPy (cupy->numpy) transfer function is unecessary applied to a numpy array.
2 participants