Add fold.jl #303

spazewalker · 2021-03-19T14:49:08Z

Adding Fold/Unfold functions in reference to pytorch feature parity here.

CarloLucibello · 2021-03-19T15:47:09Z

from a quick glance looks mostly ok, thanks. Convenience methods for the 1d and 2d cases are missing.
Then some tests and rrules

DhairyaLGandhi

Thanks so much to look into this! I've left a couple thoughts but overall I'm wondering if you've checked this on the GPU as well

DhairyaLGandhi · 2021-03-19T15:46:05Z

src/fold.jl

+    cdims = DenseConvDims(x_dim, w_dim; stride=stride, padding=padding, dilation=dilation)
+
+    # Calculate the total number of sliding blocks
+    col_dim = (im2col_dims(cdims))[1:2]


What does the 1:2 stand for here? Are you trying to extract the features from eg the channel separation/ batching dimension?

im2col_dims() returns a Tuple of 3 elements (L, M, N) where N is the number of thread. However, im2col!() assumes a 2d array col in the shape of (L, M). Hence the [1:2], to take only the first two.

DhairyaLGandhi · 2021-03-19T15:46:37Z

src/fold.jl

+    # Iterate through all batchs
+    for i = 1:x_dim[end]
+        temp = fill(0., col_dim)
+        im2col!(temp, X[:,:,:,:,i], cdims)


This would allocate the datapoint everytime, maybe use views?

DhairyaLGandhi · 2021-03-19T15:47:34Z

src/fold.jl

+
+    # Calculate the total number of sliding blocks
+    col_dim = (im2col_dims(cdims))[1:2]
+    col = undef


I'm a little uncomfortable using undef since it have erratic behaviour in corner cases.

Noted. I'll instead use fill() to initialise and then use views to fill it with actual values in the loop.

DhairyaLGandhi · 2021-03-19T15:49:15Z

src/fold.jl

+    col = undef
+
+    # Iterate through all batchs
+    for i = 1:x_dim[end]


We might want to consider the possibility of having some other dimsension to fold over as well. Not critical though

x_dim[end] is the number of batches. This loop iterates over all batches and unfolds them, and then stack each unfolded array on top of the previous one along a new dimension. Folding over other dimensions can be implemented on top of this implementation by adding a dummy singleton dimension.

DhairyaLGandhi · 2021-03-19T15:49:24Z

src/fold.jl

+        if i == 1
+            col = reshape(temp, col_dim[1], col_dim[2], 1)
+        else
+            col = cat(dims=3, col, temp)


I'll drop this if-else clause and use views here too.

DhairyaLGandhi · 2021-03-19T15:49:55Z

src/fold.jl

+            col = cat(dims=3, col, temp)
+        end
+    end
+    return col;


Prefer to drop the return, and there is a trailing semicolon

I'll remove the semicolon and are you talking about dropping the return statement or the return character?

spazewalker

from a quick glance looks mostly ok, thanks. Convenience methods for the 1d and 2d cases are missing.
Then some tests and rrules

Should I implement them here or will they be taken care of while implementing a layer in Flux.jl.

spazewalker · 2021-03-19T17:02:05Z

src/fold.jl

+    cdims = DenseConvDims(x_dim, w_dim; stride=stride, padding=padding, dilation=dilation)
+
+    # Calculate the total number of sliding blocks
+    col_dim = (im2col_dims(cdims))[1:2]


im2col_dims() returns a Tuple of 3 elements (L, M, N) where N is the number of thread. However, im2col!() assumes a 2d array col in the shape of (L, M). Hence the [1:2], to take only the first two.

spazewalker · 2021-03-19T17:02:07Z

src/fold.jl

+
+    # Calculate the total number of sliding blocks
+    col_dim = (im2col_dims(cdims))[1:2]
+    col = undef


Noted. I'll instead use fill() to initialise and then use views to fill it with actual values in the loop.

spazewalker · 2021-03-19T17:02:08Z

src/fold.jl

+    col = undef
+
+    # Iterate through all batchs
+    for i = 1:x_dim[end]


x_dim[end] is the number of batches. This loop iterates over all batches and unfolds them, and then stack each unfolded array on top of the previous one along a new dimension. Folding over other dimensions can be implemented on top of this implementation by adding a dummy singleton dimension.

spazewalker · 2021-03-19T17:02:10Z

src/fold.jl

+    # Iterate through all batchs
+    for i = 1:x_dim[end]
+        temp = fill(0., col_dim)
+        im2col!(temp, X[:,:,:,:,i], cdims)


spazewalker · 2021-03-19T17:02:12Z

src/fold.jl

+        if i == 1
+            col = reshape(temp, col_dim[1], col_dim[2], 1)
+        else
+            col = cat(dims=3, col, temp)


I'll drop this if-else clause and use views here too.

spazewalker · 2021-03-19T17:02:14Z

src/fold.jl

+            col = cat(dims=3, col, temp)
+        end
+    end
+    return col;


I'll remove the semicolon and are you talking about dropping the return statement or the return character?

spazewalker · 2021-03-19T17:13:00Z

Thanks so much to look into this! I've left a couple thoughts but overall I'm wondering if you've checked this on the GPU as well

I haven't tested it on GPU yet, I'll check it once.

DhairyaLGandhi · 2021-03-19T17:17:55Z

We might not have a layer in or maybe we need to just hold the config to construct the cdims object.

ToucheSir · 2021-03-19T17:18:38Z

from a quick glance looks mostly ok, thanks. Convenience methods for the 1d and 2d cases are missing.
Then some tests and rrules

Should I implement them here or will they be taken care of while implementing a layer in Flux.jl.

Thanks for the PR! Tests and rrules need to be here. I'm not sure what exactly is meant by convenience methods, but if it makes sense to call directly (as opposed to only as a helper for a layer struct) then it should be in NNlib as well.

spazewalker · 2021-03-19T17:33:34Z

We might not have a layer in or maybe we need to just hold the config to construct the cdims object.

Okay, so I'll try to implement those convenience wrappers.

from a quick glance looks mostly ok, thanks. Convenience methods for the 1d and 2d cases are missing.
Then some tests and rrules

Should I implement them here or will they be taken care of while implementing a layer in Flux.jl.

Thanks for the PR! Tests and rrules need to be here. I'm not sure what exactly is meant by convenience methods, but if it makes sense to call directly (as opposed to only as a helper for a layer struct) then it should be in NNlib as well.

As per my understanding, Those are the wrappers around this implementation, so that it can handle 1d and 2d inputs too. I'll work on tests but I'm not sure what 'rrule`s are.

ToucheSir · 2021-03-19T17:56:47Z

rrules are what allow for the AD system and gradients to work, more details at https://juliadiff.org/ChainRulesCore.jl/stable/#frule-and-rrule. For functions like this that use mutation and inner loops, you'll probably need another function that manually performs the backwards pass. They're a little abstracted, but it may be worth looking at how the conv rrules work:

NNlib.jl/src/conv.jl

Line 223 in 7b8ae45

@eval function rrule(::typeof($conv), x, w, cdims; kw...)

.

DhairyaLGandhi · 2021-03-28T15:51:58Z

src/fold.jl

+    # Create DenseConvDims object
+    col_dim = size(col)
+    channels = col_dim[2]÷prod(w_dim)
+    x_dim = (out_dim... , fill(3-length(out_dim))... , channels,col_dim[3])


The calls to fill seem suspect. Did you need ntuple there?

needed a ntuple with dimensions of out_dim appended with dummy dimension to make it 3d. I think it should be fill(1,3-length(out_dim)). It's okay to use fill here right?

Use insert_singleton_dimension instead. Or a reshape. fill is incorrect here.

DhairyaLGandhi · 2021-03-28T15:52:55Z

src/fold.jl

+    end
+
+    return reshape(img, (out_dim... , channels,col_dim[3]))
+end


Definitely don't want to see this. Possibly the editor is using file endings different from Linux. I'd check that.

DhairyaLGandhi · 2021-03-28T16:06:53Z

src/fold.jl

@@ -0,0 +1,70 @@
+export unfold, fold


Let's not export them, the names would dirty the name space very quickly.

mcabbott · 2022-11-28T17:00:49Z

Replaced by #444

Add fold.jl

a886db5

DhairyaLGandhi reviewed Mar 19, 2021

View reviewed changes

spazewalker commented Mar 19, 2021

View reviewed changes

Add support for 1d and 2d images in fold.jl

0380324

DhairyaLGandhi reviewed Mar 28, 2021

View reviewed changes

nikopj mentioned this pull request Nov 23, 2022

Add fold and unfold #444

Merged

2 tasks

mcabbott closed this Nov 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fold.jl #303

Add fold.jl #303

spazewalker commented Mar 19, 2021

CarloLucibello commented Mar 19, 2021

DhairyaLGandhi left a comment

DhairyaLGandhi Mar 19, 2021

spazewalker Mar 19, 2021

DhairyaLGandhi Mar 19, 2021

spazewalker Mar 19, 2021

DhairyaLGandhi Mar 19, 2021

spazewalker Mar 19, 2021

DhairyaLGandhi Mar 19, 2021

spazewalker Mar 19, 2021

DhairyaLGandhi Mar 19, 2021

spazewalker Mar 19, 2021

DhairyaLGandhi Mar 19, 2021

spazewalker Mar 19, 2021

spazewalker left a comment •

edited

Loading

spazewalker Mar 19, 2021

spazewalker Mar 19, 2021

spazewalker Mar 19, 2021

spazewalker Mar 19, 2021

spazewalker Mar 19, 2021

spazewalker Mar 19, 2021

spazewalker commented Mar 19, 2021 •

edited

Loading

DhairyaLGandhi commented Mar 19, 2021

ToucheSir commented Mar 19, 2021

spazewalker commented Mar 19, 2021

ToucheSir commented Mar 19, 2021

DhairyaLGandhi Mar 28, 2021

spazewalker Mar 28, 2021

DhairyaLGandhi Mar 28, 2021

DhairyaLGandhi Mar 28, 2021

DhairyaLGandhi Mar 28, 2021

mcabbott commented Nov 28, 2022

Add fold.jl #303

Add fold.jl #303

Conversation

spazewalker commented Mar 19, 2021

CarloLucibello commented Mar 19, 2021

DhairyaLGandhi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spazewalker left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spazewalker commented Mar 19, 2021 • edited Loading

DhairyaLGandhi commented Mar 19, 2021

ToucheSir commented Mar 19, 2021

spazewalker commented Mar 19, 2021

ToucheSir commented Mar 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcabbott commented Nov 28, 2022

spazewalker left a comment •

edited

Loading

spazewalker commented Mar 19, 2021 •

edited

Loading