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

GRADIENT_DESCENT does not work as expected #8393

Open
mshafiei opened this issue Aug 20, 2024 · 10 comments
Open

GRADIENT_DESCENT does not work as expected #8393

mshafiei opened this issue Aug 20, 2024 · 10 comments
Assignees

Comments

@mshafiei
Copy link

Hi,

I'm trying to get the gradient of local laplacian generator. When I add GRADIENT_DESCENT_OPT "GRADIENT_DESCENT" to python_bindings/apps/CMakeLists.txt:48 the local laplacian app breaks. The output image turns out to be pitch black. Also the gradient image is black (you need to add an additional buffer as the last parameter list of the generator in the local_laplacian_app.py). Is there any workaround for this issue?

@abadams
Copy link
Member

abadams commented Aug 21, 2024

Sounds like something is broken, but before investigating I wanted to point out that the gradient w.r.t. the input image is going to be zero for that algorithm anyway, because it starts with a nearest-neighbor-sampled lookup table.

@mshafiei
Copy link
Author

I'm actually interested in the derivative w.r.t. alpha. I think that doesn't rely on the nearest neighbor look up. Is that correct?
I also tried gradient on other generators e.g. blur, bilateral grid but none of them seem to work.

@mshafiei mshafiei changed the title Local laplacian gradient does not work GRADIENT_DESCENT_OPT does not work as expected Nov 26, 2024
@mshafiei
Copy link
Author

The issue happens for any generator (I've tried LLF, blur, and bilateral filtering). Follow the steps below to reproduce:

  1. Add GRADIENT_DESCENT_OPT "GRADIENT_DESCENT" to add_halide_library in ${HALIDE_PATH}/python_bindings/apps/CMakeLists.txt.
  2. Build Halide
  3. Modify build_app.py, store the gradient buffer in an image using grad_buf_u8 = grad_buf.astype(np.uint8) and
    halide.imageio.imwrite(grad_path, grad_buf_u8)
  4. Gradient image is black
  5. Output image is also corrupted

@alexreinking
Copy link
Member

Please consult the documentation for add_halide_library: https://github.com/halide/Halide/blob/main/doc/HalideCMakePackage.md#add_halide_library

The option is GRADIENT_DESCENT, no _OPT suffix, and it does not accept an argument. For example:

add_halide_library(
  lib FROM gen
  GRADIENT_DESCENT
)

@mshafiei
Copy link
Author

Thanks @alexreinking. I think CMake still picked up GRADIENT_DESCENT and the unnecessary GRADIENT_DESCENT_OPT that I have specified was ignored. So the issue still exists. This code snippet reproduces it.

from blur import blur
import halide.imageio
import numpy as np
import sys
import timeit


if __name__ == "__main__":
    input_path = sys.argv[1]
    output_path = sys.argv[2]
    grad_path = sys.argv[3]
    timing_iterations = 10

    print("Reading from %s ..." % input_path)
    input_buf_u8 = halide.imageio.imread(input_path)[0,:,:]
    assert input_buf_u8.dtype == np.uint8
    
    # Convert to uint16... but remember that the blur() generator
    # is documented as only working on <= 14 bits of image; if
    # we use the upper two bits we'll get incorrect results.
    # We'll just leave it with 8 bits of useful data.
    input_buf = input_buf_u8.astype(np.uint16)
    output_buf = np.empty(input_buf.shape, dtype=input_buf.dtype)
    grad_buf = np.empty(input_buf.shape, dtype=input_buf.dtype)

    blur(input_buf, output_buf, grad_buf)

    output_buf_u8 = output_buf.astype(np.uint8)
    
    print("Saving to %s ..." % output_path)
    halide.imageio.imwrite(output_path, output_buf_u8)

    print("Saving to %s ..." % grad_path)
    grad_buf_u8 = grad_buf.astype(np.uint8)
    halide.imageio.imwrite(grad_path, grad_buf_u8)

@mshafiei mshafiei changed the title GRADIENT_DESCENT_OPT does not work as expected GRADIENT_DESCENT does not work as expected Nov 27, 2024
@mshafiei
Copy link
Author

@alexreinking @abadams Can you reproduce the issue with the code snippet above?

@alexreinking
Copy link
Member

We're all out this week for Thanksgiving. I can take a look next week!

@abadams
Copy link
Member

abadams commented Dec 17, 2024

Bump. I don't feel qualified to look at this one, because it's probably something related to buffer management in the python layer.

@alexreinking alexreinking self-assigned this Dec 18, 2024
@steven-johnson steven-johnson self-assigned this Dec 18, 2024
@steven-johnson
Copy link
Contributor

steven-johnson commented Dec 19, 2024

FWIW, enabling GRADIENT_DESCENT for the Python apps in this way seems to take ~forever to compile -- investigating

EDIT: building in verbose mode reveals Warning: Autoscheduling is not enabled in build_gradient_module(), so the resulting gradient module will be unscheduled; this is very unlikely to be what you want.

@steven-johnson
Copy link
Contributor

The option is GRADIENT_DESCENT, no _OPT suffix

Why didn't this produce an error at CMake configure time? Surely unknown keywords should fail.

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

No branches or pull requests

4 participants