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

Add decompress module for prototype cupy backend #4962

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

GarethCabournDavies
Copy link
Contributor

For the CuPy backend, we need the decompress module translated; this is that translation

Standard information about the request

This is a new feature
This change affects the offline search, as I don't think any others (use compressed waveforms)
This change changes scientific output
This change follows style guidelines (See e.g. PEP8), has been proposed using the contribution guidelines
This change will require additional dependencies (CuPy in the new backend)

Contents

This is effectively a copy of the decompress_cuda module, but using cupy rather than pycuda

Links to any issues or associated PRs

#4952

Testing performed

Comparison of waveforms with/without compression and from CPU/CuPy:

  • The author of this pull request confirms they will adhere to the code of conduct

@GarethCabournDavies
Copy link
Contributor Author

Testing

image

mismatches:

CuPy Not Compressed -- CuPy Compressed [0.00122166]
CuPy Not Compressed -- CPU Not Compressed [1.7881393e-07]
CuPy Not Compressed -- CPU Compressed [0.00122166]
CuPy Compressed -- CPU Not Compressed [2.8014183e-05]
CuPy Compressed -- CPU Compressed [0.]
CPU Not Compressed -- CPU Compressed [0.00122112]

This means that the compressed waveform implementation on CPU and CuPy is exactly the same

@spxiwh
Copy link
Contributor

spxiwh commented Nov 29, 2024

(@GarethCabournDavies You will need to wait for #4962 to be merged before sanity tests could pass here, or at least you'll need fixes in there (like for documentation build). I'll hold on reviewing this until #4962 is merged)

(nb,), (nt,),
(g_out, df, hlen, flow, fmax, texlen, freqs_gpu, amps_gpu, phases_gpu, lower, upper)
)
cp.cuda.runtime.deviceSynchronize()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this synchronize needed?

# Note that imin and start_index are ignored in the GPU code; they are only
# needed for CPU.
if output.precision == 'double':
raise NotImplementedError("Double precision linear interpolation not currently supported on CUDA scheme")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this be easily supported now with dynamic types in Cupy?

Copy link
Contributor

@spxiwh spxiwh left a comment

Choose a reason for hiding this comment

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

Some minor comments, but I'd be happy to merge this once the tests pass.

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.

2 participants