[WIP] Add fused types, attempt to use in denoise by AetherUnbound · Pull Request #3469 · scikit-image/scikit-image

AetherUnbound · 2018-10-11T16:40:24Z

Description

This is my first stab at adding the fused types from #3253 in order to fix the issue identified in #3449. I'm totally new to Cython as a whole, so I'm open to assistance with this one!

Checklist

Clean style in the spirit of PEP8
Docstrings for all functions
Gallery example in ./doc/examples (new features only)
Benchmark in ./benchmarks, if your changes aren't covered by an
existing benchmark
Unit tests

For reviewers

(Don't remove the checklist below.)

Check that the PR title is short, concise, and will make sense 1 year
later.
Check that new functions are imported in corresponding __init__.py.
Check that new features, API changes, and deprecations are mentioned in
doc/release/release_dev.rst.
Consider backporting the PR with @meeseeksdev backport to v0.14.x

AetherUnbound · 2018-10-11T16:52:51Z

Looking at this now, I'm seeing a lot of similarities with #3253 🤦‍♂️ specifically with interpolation.pxd. @jni, what's the best move here?

hmaarrfk · 2018-10-11T18:56:32Z

Let me cleanup that PR. you should be able to cherry-pick the commits really soon and incldue them in your PR.

hmaarrfk · 2018-10-11T19:41:14Z

@AetherUnbound Feel free to just cherry pick the commits you need from my fork.

git remote add hmaarrfk https://github.com/hmaarrfk/scikit-image.git
git fetch hmaarrfk
git cherry-pick THE_COMMITS YOU WANT

I tested the interpolation fused code with the test suite so hopefully it shouldn't cause you lots of problems.

No guarantees that it will work, but you know, if it helps.

you might also want to have a look at this PR: #3254
In case the discussion there helps you.

jni · 2018-10-11T23:05:54Z

@AetherUnbound good stuff! I think the best approach now is:

wait for Remove cython md5 hashing since it breaks the build process #3254 to get merged
cherry pick @hmaarrfk's newly-minted commits from Enable warping with more than just doubles. #3253
apply a new commit on top of those for the denoise file. You can probably cherry-pick your own commit using --strategy=ours (meaning: keep the changes from @hmaarrfk instead of your own if there is a conflict)
push that set of commits to a new PR or --force-with-lease push to this branch to update this PR

Let us know if you need any git-help!

hmaarrfk · 2018-10-12T00:22:07Z

Looking at his profile, I'm pretty sure @AetherUnbound knows how to use git but he was just being polite 😅 didn't mean to insult.

AetherUnbound · 2018-10-12T00:27:49Z

Oh no I didn't take it that way at all! Haha cherry picking is actually not something I've done before so it'll be a nice experiment for me 😉

AetherUnbound · 2018-10-12T05:04:14Z

Thanks for all the help/suggestions guys - I've cherry-picked the commits but now when I try to run cython skimage/_shared/fused_numerics.pdx I get this:

 aether   bugfix/float-cast  ~  git  scikit-image  cython skimage/_shared/fused_numerics.pxd 

Error compiling Cython file:
------------------------------------------------------------
...
cimport numpy as cnp
import numpy as np

ctypedef fused np_ints:
^
------------------------------------------------------------

skimage/_shared/fused_numerics.pxd:4:0: 'np_ints' redeclared

I'm assuming this has to do with the md5/c compiled stuff.

AetherUnbound · 2018-10-12T05:14:26Z

I've changed double to be np_floats in _denoise_cy.pxd (do I need to change instances of np.double to be something else?). The file itself compiles, but when I try and run the sample function I supplied in #3449 I get this:

In [1]: import numpy as np
   ...: import skimage.restoration
   ...: 
   ...: sigma = 16.0
   ...: sigma_range = .2
   ...: np.random.seed(0)
   ...: image = np.random.uniform(size=(100, 100)).astype(np.float32)
   ...: mask = np.ones(image.shape, bool)
   ...: mask[40:60, 45:65] = False
   ...: expected = skimage.restoration.denoise_bilateral(
   ...:     image=image,
   ...:     multichannel=False,
   ...:     sigma_color=sigma_range,
   ...:     sigma_spatial=sigma,
   ...: )
   ...: 
   ...: 


---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
~/git/scikit-image/skimage/__init__.py in <module>()
    120     try:
--> 121         from ._shared import geometry
    122         del geometry

ImportError: cannot import name 'geometry'

During handling of the above exception, another exception occurred:

ImportError                               Traceback (most recent call last)
<ipython-input-1-c4156936ec11> in <module>()
      1 import numpy as np
----> 2 import skimage.restoration
      3 
      4 sigma = 16.0
      5 sigma_range = .2

~/git/scikit-image/skimage/__init__.py in <module>()
    122         del geometry
    123     except ImportError as e:
--> 124         _raise_build_error(e)
    125 
    126     # All skimage root imports go here

~/git/scikit-image/skimage/__init__.py in _raise_build_error(e)
    102     raise ImportError("""%s
    103 It seems that scikit-image has not been built correctly.
--> 104 %s""" % (e, msg))
    105 
    106 

ImportError: cannot import name 'geometry'
It seems that scikit-image has not been built correctly.

Your install of scikit-image appears to be broken.
Try re-installing the package following the instructions at:
https://scikit-image.org/docs/stable/install.html

Even after doing a pip install -e . (is there an easier way to just recompile the files that changed?)

hmaarrfk · 2018-10-12T05:27:45Z

Can you try to remove all md5 files and compile again with python setup develop or pip install. You have to keep running those commands every change

AetherUnbound · 2018-10-12T05:56:46Z

I ran the following and am still getting the same issue with the geometry import:

 aether  (e) skimage   bugfix/float-cast  ~  git  scikit-image  find . -type f -name "*.md5" | xargs rm
 aether  (e) skimage   bugfix/float-cast  ~  git  scikit-image  pip install -e .
``

hmaarrfk · 2018-10-12T06:24:38Z

skimage/restoration/_denoise_cy.pyx


    cdef:
-        double[:] color_lut = np.empty(bins, dtype=np.double)
+        np_floats[:] color_lut = np.empty(bins, dtype=np.double)


right hand side should probably be np_floats as well.

Your choice, but I would just pop out this numpy array creation out of cython. It doesn't save much time and is annoying to write in cython. But do what you want.

Looking at this at a reasonable type, I can see why this is done in cython, these seem like implementation details of the algorithm. I think you might just need to change np.double to np_floats. For each version of the function, Cython will pick the same float for all instances of np_floats in a function.

hmaarrfk · 2018-10-12T06:24:55Z

skimage/restoration/_denoise_cy.pyx


    cdef:
-        double[:] range_lut = np.empty(win_size*win_size, dtype=np.double)
+        np_floats[:] range_lut = np.empty(win_size*win_size, dtype=np.double)


dtype on RHS

hmaarrfk · 2018-10-12T06:26:30Z

skimage/restoration/_denoise_cy.pyx

-        double[:, :, ::1] bx = np.zeros(shape_ext, dtype=np.double)
-        double[:, :, ::1] by = np.zeros(shape_ext, dtype=np.double)
+        np_floats[:, :, ::1] dx = np.zeros(shape_ext, dtype=np.double)
+        np_floats[:, :, ::1] dy = np.zeros(shape_ext, dtype=np.double)


All those dtypes on the RHS;

hmaarrfk · 2018-10-12T06:28:03Z

FYI, that MD5 PR just got pulled in. If you want you can pull upstream mster, and rebase your branch.

…lation of images.

AetherUnbound · 2018-10-12T15:07:32Z

Awesome, I've rebased and changed np.doubles to np_floats, but now I get the following error:

    Error compiling Cython file:
    ------------------------------------------------------------
    ...
    
    
    cdef np_floats[:] _compute_color_lut(Py_ssize_t bins, np_floats sigma, np_floats max_value):
    
        cdef:
            np_floats[:] color_lut = np.empty(bins, dtype=np_floats)
                                                         ^
    ------------------------------------------------------------
    
    skimage/restoration/_denoise_cy.pyx:22:54: 'np_floats' is not a constant, variable or function identifier
    
    Error compiling Cython file:
    ------------------------------------------------------------
    ...
    
    
    cdef np_floats[:] _compute_color_lut(Py_ssize_t bins, np_floats sigma, np_floats max_value):
    
        cdef:
            np_floats[:] color_lut = np.empty(bins, dtype=np_floats)
                                                         ^
    ------------------------------------------------------------
    
    skimage/restoration/_denoise_cy.pyx:22:54: Type is not specialized

hmaarrfk · 2018-10-12T15:25:39Z

Rightttttt so that was the real reason why I recommended against using np.empty in cython. My only way around this was
https://cython.readthedocs.io/en/latest/src/userguide/fusedtypes.html#type-checking-specializations

where by I had different branches for each type I wanted to support. I just don't think it is possible to do this without this kind of hack.

For me, it was only used for the output array of warp. Therefore, I moved that to python, and didn't deal with it again.

For you, depending on if you think it be best to continue this in Cython, it might look something like:

cdef np_floats[:, :, ::1] np_empty_float(shape_type???? shape, np_floats d):
    if np_floats is np.float32:
          return np.empty(shape, np.float32)
    [ .... ]

AetherUnbound · 2018-10-12T16:22:46Z

Thanks @hmaarrfk! I'm not sure how I'd refactor denoise to pull out all of the np.empty or np.zeros calls.

I've given it a stab in cython:


cdef inline np_floats[:] _np_empty(Py_ssize_t shape, np_floats dtype):
    if dtype is np.float32:
        return np.empty(shape, np.float32)
    else:
        return np.empty(shape, np.float64)


cdef inline np_floats[:, :, ::1] _np_zeros(Py_ssize_t shape, np_floats dtype):
    if dtype is np.float32:
        return np.zeros(shape, np.float32)
    else:
        return np.zeros(shape, np.float64)


cdef np_floats[:] _compute_color_lut(Py_ssize_t bins, np_floats sigma, np_floats max_value):

    cdef:
        np_floats[:] color_lut = _np_empty(bins, np_floats)
        Py_ssize_t b

    sigma *= sigma
    max_value /= bins

    for b in range(bins):
        color_lut[b] = _gaussian_weight(sigma, b * max_value)

    return color_lut

But I'm getting the following errors:

Error compiling Cython file:
------------------------------------------------------------
...


cdef np_floats[:] _compute_color_lut(Py_ssize_t bins, np_floats sigma, np_floats max_value):

    cdef:
        np_floats[:] color_lut = _np_empty(bins, np_floats[:])
                                                ^
------------------------------------------------------------

skimage/restoration/_denoise_cy.pyx:36:49: 'np_floats' is not a constant, variable or function identifier

Error compiling Cython file:
------------------------------------------------------------
...


cdef np_floats[:] _compute_color_lut(Py_ssize_t bins, np_floats sigma, np_floats max_value):

    cdef:
        np_floats[:] color_lut = _np_empty(bins, np_floats[:])
                                                ^
------------------------------------------------------------

skimage/restoration/_denoise_cy.pyx:36:49: Type is not specialized

Error compiling Cython file:
------------------------------------------------------------
...


cdef np_floats[:] _compute_color_lut(Py_ssize_t bins, np_floats sigma, np_floats max_value):

    cdef:
        np_floats[:] color_lut = _np_empty(bins, np_floats[:])
                                         ^
------------------------------------------------------------

skimage/restoration/_denoise_cy.pyx:36:42: no suitable method found

(Sorry I'm not of more help here 😭)

hmaarrfk · 2018-10-12T16:33:47Z

Dtype is a real hack. You should basically pass sigma in.

AetherUnbound · 2018-10-12T17:15:06Z

Got it, I'm passing in sigma now to _np_empty and that's working fine. However, the _np_zeros function is throwing no suitable method found for every call that's being made. Here's the function:

cdef inline np_floats[:, :, ::1] _np_zeros(Py_ssize_t shape, np_floats dtype):
    if dtype is np.float32:
        return np.zeros(shape, np.float32)
    else:
        return np.zeros(shape, np.float64)

And an example call:

def _denoise_bilateral(image, Py_ssize_t win_size, sigma_color,
                      np_floats sigma_spatial, Py_ssize_t bins,
                      mode, np_floats cval):
    cdef:
        np_floats min_value, max_value

    min_value = image.min()
    max_value = image.max()

    if min_value == max_value:
        return image

    # if image.max() is 0, then dist_scale can have an unverified value
    # and color_lut[<int>(dist * dist_scale)] may cause a segmentation fault
    # so we verify we have a positive image and that the max is not 0.0.
    if min_value < 0.0:
        raise ValueError("Image must contain only positive values")

    if max_value == 0.0:
        raise ValueError("The maximum value found in the image was 0.")

    image = np.atleast_3d(img_as_float(image))

    cdef:
        Py_ssize_t rows = image.shape[0]
        Py_ssize_t cols = image.shape[1]
        Py_ssize_t dims = image.shape[2]
        Py_ssize_t window_ext = (win_size - 1) / 2
        Py_ssize_t max_color_lut_bin = bins - 1

        np_floats[:, :, ::1] cimage
        np_floats[:, :, ::1] out

        np_floats[:] color_lut
        np_floats[:] range_lut

        Py_ssize_t r, c, d, wr, wc, kr, kc, rr, cc, pixel_addr, color_lut_bin
        np_floats value, weight, dist, total_weight, csigma_color, color_weight, \
               range_weight, t
        np_floats dist_scale
        np_floats[:] values
        np_floats[:] centres
        np_floats[:] total_values

    if sigma_color is None:
        csigma_color = image.std()
    else:
        csigma_color = sigma_color

    if mode not in ('constant', 'wrap', 'symmetric', 'reflect', 'edge'):
        raise ValueError("Invalid mode specified.  Please use `constant`, "
                         "`edge`, `wrap`, `symmetric` or `reflect`.")
    cdef char cmode = ord(mode[0].upper())

    cimage = np.ascontiguousarray(image)

    out = _np_zeros((rows, cols, dims), sigma_spatial)

And the compiler error:

Error compiling Cython file:
------------------------------------------------------------
...
                         "`edge`, `wrap`, `symmetric` or `reflect`.")
    cdef char cmode = ord(mode[0].upper())

    cimage = np.ascontiguousarray(image)

    out = _np_zeros((rows, cols, dims), sigma_spatial)
                  ^
------------------------------------------------------------

skimage/restoration/_denoise_cy.pyx:131:19: no suitable method found

This make me believe that I don't have the types defined in the inline function correctly, but it works for the np.empty function :/

AetherUnbound · 2018-10-12T17:34:05Z

Got it - I totally missed that _np_zeros is returning a 3D array and should thus receive its shape as a tuple and not a Py_ssize_t. Working on that now.

AetherUnbound · 2018-10-12T18:32:14Z

Turns out I wasn't close at all (cause I was checking if the parameter passed in was np.float32, not if it's type was np.float32). Here's where I'm at now:

cdef inline np_floats[:] _np_empty(Py_ssize_t size, np_floats dtype):
    if np_floats is np.float32:
        return np.empty(size, np.float32)
    else:
        return np.empty(size, np.float64)


cdef inline np_floats[:, :, ::1] _np_zeros(tuple shape, np_floats[:, :, ::1] dtype):
    if np_floats is np.float32:
        return np.zeros(shape, np.float32)
    else:
        return np.zeros(shape, np.double)

With error:

Error compiling Cython file:
------------------------------------------------------------
...
cdef inline np_floats _gaussian_weight(np_floats sigma_sqr, np_floats value):
    return exp(-0.5 * value * value / sigma_sqr)


cdef inline np_floats[:] _np_empty(Py_ssize_t size, np_floats dtype):
    if np_floats is np.float32:
      ^
------------------------------------------------------------

skimage/restoration/_denoise_cy.pyx:20:7: 'np_floats' is not a constant, variable or function identifier

Error compiling Cython file:
------------------------------------------------------------
...
cdef inline np_floats _gaussian_weight(np_floats sigma_sqr, np_floats value):
    return exp(-0.5 * value * value / sigma_sqr)


cdef inline np_floats[:] _np_empty(Py_ssize_t size, np_floats dtype):
    if np_floats is np.float32:
      ^
------------------------------------------------------------

skimage/restoration/_denoise_cy.pyx:20:7: Type is not specialized

hmaarrfk

Personally, I would keep the sentinel values scalar. If you need to specify them using cimage then index the first entry.

The advantage would be that you could put them in the fused types headers for others to use in the future.

jni · 2018-10-13T01:33:14Z

I'm going to go ahead and second @hmaarrfk's original sentiment that you should aim to make all allocations in Python and then pass both input and output arrays to Cython. That will greatly simplify the code, as maintaining input dtypes in Python is super easy: lut = np.empty(shape, dtype=image.dtype); out=np.zeros_like(image).

jni · 2018-10-13T01:33:39Z

And of course there is virtually zero performance benefit in doing this in Cython.

AetherUnbound · 2018-10-13T03:46:32Z

I like that idea! The trouble is that there's all these other arrays that should also be zero (https://github.com/scikit-image/scikit-image/blob/master/skimage/restoration/_denoise_cy.pyx#L116-L122), but this is all done with shape calculations based on the atleast_3d'd array: https://github.com/scikit-image/scikit-image/blob/master/skimage/restoration/_denoise_cy.pyx#L81. Since most of that block is just allocating arrays and calculating shapes, should it all be moved into python?

jni · 2018-10-13T05:09:37Z

@AetherUnbound yes, feel free to “refactor with abandon”, as a friend once put it. That’s what the tests are for! =) (And of course, at the end of it, please add a test with a float32 image as input!)

jni · 2018-10-15T03:35:35Z

🎉 this is all super-awesome! However, we have an ominous looking error in AppVeyor:

python35-x64/lib/site-packages/skimage/restoration/tests/test_denoise.py::test_denoise_tv_bregman_2d Command exited with code -1073741819

https://ci.appveyor.com/project/scikit-image/scikit-image/builds/19505999/job/549juoaxo0lgn1mt#L8039

jni · 2018-10-15T03:35:47Z

Or you can ignore me if that last commit fixes it. =P

AetherUnbound · 2018-10-15T03:36:13Z

@jni it likely doesn't and I don't know why that error is occurring :<

jni · 2018-10-15T03:40:41Z

Ouch. This seems to suggest that we need to acquire the GIL for some of the code:

https://stackoverflow.com/questions/38973604/process-finished-with-exit-code-1073741819-0xc0000005-cython-teamspeak3

Just to double check: tests run fine on your machine?

jni · 2018-10-15T03:42:05Z

I take it back: the comments under the selected answer suggest that the answer made things worse! Why is the answer ticked??? 😂

AetherUnbound · 2018-10-15T03:42:34Z

Yea, they do work on my machine!

 aether  (e) skimage   bugfix/float-cast  ~  git  scikit-image  pytest skimage/restoration/tests/test_denoise.py 
============================================================================================================ test session starts =============================================================================================================
platform linux -- Python 3.7.0, pytest-3.8.2, py-1.7.0, pluggy-0.7.1
rootdir: /home/aether/git/scikit-image/skimage/restoration, inifile:
plugins: cov-2.6.0
collected 39 items                                                                                                                                                                                                                           

skimage/restoration/tests/test_denoise.py .......................................                                                                                                                                                      [100%]

============================================================================================================== warnings summary ==============================================================================================================
/home/aether/programs/anaconda3/envs/skimage/lib/python3.7/site-packages/dask/config.py:12: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated, and in 3.8 it will stop working
  from collections import Mapping

/home/aether/programs/anaconda3/envs/skimage/lib/python3.7/site-packages/pywt/_utils.py:6: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated, and in 3.8 it will stop working
  from collections import Iterable

-- Docs: https://docs.pytest.org/en/latest/warnings.html
=================================================================================================== 39 passed, 2 warnings in 5.47 seconds ====================================================================================================```

jni · 2018-10-15T03:44:49Z

This other thread suggests that the error code means "memory corruption", which indeed sounds so ominous! Gah!

explosion/spaCy#799

Anyway let me play around with it... Thank you very much for your efforts!

AetherUnbound · 2018-10-15T03:47:20Z

Okay, let me know if there's anything else I can do - thanks!

hmaarrfk · 2018-10-15T04:12:10Z

skimage/restoration/_denoise_cy.pyx


    shape_ext = (rows2, cols2, dims)
-    u = np.zeros(shape_ext, dtype=np.double)
+    u = out.copy()


you should explain what this is. Can you call np.zeros_like instead?

I can definitely add a comment here

hmaarrfk · 2018-10-15T04:17:14Z

This kinda seems silly:

return np.squeeze(np.asarray(out))

The asarray is not necessary.

hmaarrfk · 2018-10-15T04:24:52Z

    # reflect image
    u[0, 1:-1] = image[1, :]
    u[1:-1, 0] = image[:, 1]
    u[-1, 1:-1] = image[-2, :]
    u[1:-1, -1] = image[:, -2]

We can't have negative indices.

hmaarrfk · 2018-10-15T04:36:15Z

Nevermind, I don't like zeros_like anymore
numpy/numpy#9909

AetherUnbound · 2018-10-15T05:10:06Z

    # reflect image
    u[0, 1:-1] = image[1, :]
    u[1:-1, 0] = image[:, 1]
    u[-1, 1:-1] = image[-2, :]
    u[1:-1, -1] = image[:, -2]

We can't have negative indices.

The operational stuff in this isn't my code - what would you suggest here?

hmaarrfk · 2018-10-15T05:23:09Z

Fix it, keep it as a seperate commit if you wish for you PR not to be squashed. If @jni wants, we can think of making a seperate PR, but maybe this is ok to bundle?

AetherUnbound · 2018-10-15T05:27:20Z

What I mean to say is, how would I change this to not use negative indices? And why are negative indices bad in the first place? 😮

hmaarrfk · 2018-10-15T05:32:53Z

#cython: wraparound=False

disables negative indices in cython.

u[1:rows-1, 1:cols-1] = image

might work

jni · 2018-10-15T14:23:58Z

Hooray for platform-specific failures!

 ~/projects/scikit-image (bugfix/float-cast)
 $ pytest skimage/restoration/tests/test_denoise.py 
==================================================== test session starts =====================================================
platform linux -- Python 3.6.3, pytest-3.5.1, py-1.5.3, pluggy-0.6.0
rootdir: /home/jni/projects/scikit-image/skimage/restoration, inifile:
plugins: cov-2.5.1, hypothesis-3.56.9
collected 39 items                                                                                                           

skimage/restoration/tests/test_denoise.py ........F..............................                                      [100%]

========================================================== FAILURES ==========================================================
_________________________________________ test_denoise_tv_bregman_float_result_range _________________________________________

    def test_denoise_tv_bregman_float_result_range():
        # astronaut image
        img = astro_gray.copy()
        int_astro = np.multiply(img, 255).astype(np.uint8)
        assert_(np.max(int_astro) > 1)
        denoised_int_astro = restoration.denoise_tv_bregman(int_astro, weight=60.0)
        # test if the value range of output float data is within [0.0:1.0]
        assert_(denoised_int_astro.dtype == np.float)
>       assert_(np.max(denoised_int_astro) <= 1.0)
E       AssertionError

skimage/restoration/tests/test_denoise.py:152: AssertionError
============================================ 1 failed, 38 passed in 5.10 seconds =============================================

sigh, gonna head to bed now, I'll try to figure it out tomorrow.

AetherUnbound · 2018-10-15T15:23:21Z

@jni that's so weird because I'm running very similar versions on linux and it was working O_o

hmaarrfk · 2018-10-15T15:26:44Z

I really think this is the negative indexing guys. Your basically using memory your not supposed to.

AetherUnbound · 2018-10-15T15:29:52Z

Okay @hmaarrfk, I'll try to address that today!

hmaarrfk · 2018-10-15T15:38:31Z

I was just saying that it seems more like a memory out of bounds problem, than a OS Specific one seeing that we found a place where negative indices were used.

Sometimes, the order in which memory is allocated causes these problems to pop up.

Finally, using different OSs makes use of different memory allocators as well, which also causes these problems to surface occasionally.

We've been able to use 32bit linux builds to spot a few other problems in Cython, mostly related to using int64/int32 as pointers when we weren't supposed to.

jni · 2018-10-16T00:00:36Z

@hmaarrfk 🤦‍♂️

Ok but I still see @AetherUnbound's point: as far as I can tell, the diff doesn't touch the Cython wraparound call or the negative indexing, so why was it working before. Was it working before??

Also, should we just create u in Python with np.pad?

hmaarrfk · 2018-10-16T00:44:34Z

np pad PR is held up. Basically, there is a trade off between high level python and straight concatenation because it is implemented in C. I wouldn’t use pad or block for these well posed problems.

…

On Mon, Oct 15, 2018 at 8:00 PM Juan Nunez-Iglesias < ***@***.***> wrote: @hmaarrfk <https://github.com/hmaarrfk> 🤦‍♂️ Ok but I still see @AetherUnbound <https://github.com/AetherUnbound>'s point: as far as I can tell, the diff doesn't touch the Cython wraparound call or the negative indexing, so why was it working before. *Was it* working before?? Also, should we just create u in Python with np.pad? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3469 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFfmBMALj-BIss0zsuVjA95x6FCFp_Vks5ulSGlgaJpZM4XX75i> .

hmaarrfk · 2018-10-16T03:55:05Z

Might be relevant to the negative index bug: cython/cython#2539

hmaarrfk · 2018-10-20T14:16:53Z

@AetherUnbound I think you should just continue your work on this PR.

It seems that the intermittent failer was caused because the range_lut was never initialized.

In finding that bug, it went through the code again and a few places that could be cleaned up:

Your cython functions should either return a value, or mutate out. They shouldn't do both like numpy does. If they do, we need explicit tests that test both code paths.
- You will have to call squeeze in python when this change is done.
In _denoise_tv_bregman, u, cu and out are all the same variable. Just remove u and cu and rename other variables that refer to u as referring to out
When you moved _compute_color_lut to python, it still looks like cython code. It should either be moved back to cython, or translated to python. A similar comment holds if you want to move the range_lut calculation to python as well.

AetherUnbound · 2018-11-11T00:21:39Z

Sorry I've let this sit for so long! I want to jump back in but it looks like there's some conflicts, @hmaarrfk any advice on how to resolve them?

jni · 2019-01-15T09:55:20Z

Closing in favour of #3486

AetherUnbound force-pushed the bugfix/float-cast branch from 2231386 to 4dabf7f Compare October 12, 2018 04:54

hmaarrfk reviewed Oct 12, 2018

View reviewed changes

hmaarrfk added 2 commits October 12, 2018 07:53

MNT: Add a fused numeric type to make fused_types more constent.

6fed1c5

MNT: make the interpolation use fused types for any->any type interpo…

7897b92

…lation of images.

AetherUnbound force-pushed the bugfix/float-cast branch from 926b499 to ae21ab6 Compare October 12, 2018 14:53

hmaarrfk reviewed Oct 12, 2018

View reviewed changes

hmaarrfk reviewed Oct 15, 2018

View reviewed changes

jni mentioned this pull request Oct 15, 2018

Improvement: Add doc notes on setting up the build environment #3472

Merged

4 tasks

jni mentioned this pull request Oct 19, 2018

Use fused types in denoise, warp #3486

Merged

9 tasks

jni closed this Jan 15, 2019

Uh oh!

Conversation

AetherUnbound commented Oct 11, 2018

Description

Checklist

For reviewers

Uh oh!

AetherUnbound commented Oct 11, 2018

Uh oh!

hmaarrfk commented Oct 11, 2018

Uh oh!

hmaarrfk commented Oct 11, 2018

Uh oh!

jni commented Oct 11, 2018

Uh oh!

hmaarrfk commented Oct 12, 2018

Uh oh!

AetherUnbound commented Oct 12, 2018

Uh oh!

AetherUnbound commented Oct 12, 2018

Uh oh!

AetherUnbound commented Oct 12, 2018

Uh oh!

hmaarrfk commented Oct 12, 2018

Uh oh!

AetherUnbound commented Oct 12, 2018

Uh oh!

hmaarrfk Oct 12, 2018

Choose a reason for hiding this comment

Uh oh!

hmaarrfk Oct 12, 2018

Choose a reason for hiding this comment

Uh oh!

hmaarrfk Oct 12, 2018

Choose a reason for hiding this comment

Uh oh!

hmaarrfk Oct 12, 2018

Choose a reason for hiding this comment

Uh oh!

hmaarrfk Oct 12, 2018

Choose a reason for hiding this comment

Uh oh!

hmaarrfk commented Oct 12, 2018

Uh oh!

AetherUnbound commented Oct 12, 2018

Uh oh!

hmaarrfk commented Oct 12, 2018

Uh oh!

AetherUnbound commented Oct 12, 2018

Uh oh!

hmaarrfk commented Oct 12, 2018

Uh oh!

AetherUnbound commented Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AetherUnbound commented Oct 12, 2018

Uh oh!

AetherUnbound commented Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hmaarrfk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jni commented Oct 13, 2018

Uh oh!

jni commented Oct 13, 2018

Uh oh!

AetherUnbound commented Oct 13, 2018

Uh oh!

jni commented Oct 13, 2018

Uh oh!

jni commented Oct 15, 2018

Uh oh!

jni commented Oct 15, 2018

Uh oh!

AetherUnbound commented Oct 15, 2018

Uh oh!

jni commented Oct 15, 2018

Uh oh!

AetherUnbound commented Oct 12, 2018 •

edited

Loading

AetherUnbound commented Oct 12, 2018 •

edited

Loading

hmaarrfk left a comment •

edited

Loading