Add support for Cropping1D and Cropping2D

[jon-tow]

Adding support for Cropping1D and Cropping2D along with tests for correctness. #54

Currently only supports cropping based on a channels last data format (Keras's default), until control flow differentiation is added.

[Shashi456]

@jon-tow Cropping is a convolution operation, would suggest moving it to convolution.swift instead of making a new file.

[jon-tow]

Oops. I assumed since it isn't a type of convolution I'd create a separate file. I will stick it in Convolution.swift as I see that's how Keras handles it. Is this correct @rxwei?

Thanks for the heads up @Shashi456.

[eaplatanios]

@Shashi456 Why do you believe that cropping is a convolution operation? I would just put it in a new file called Manipulation.swift or Image.swift. Manipulation.swift could also contains more tensor manipulation operations (e.g., reshape, tiling, ...).

[Shashi456]

@eaplatanios, just mostly following the keras api structure for now.

[rxwei]

@Shashi456 Why do you believe that cropping is a convolution operation? I would just put it in a new file called Manipulation.swift or Image.swift. Manipulation.swift could also contains more tensor manipulation operations (e.g., reshape, tiling, ...).

I agree with Anthony here. Image.swift sounds like a good name.

[Shashi456]

Can we have a separate PR for refactoring the layers into image.swift? And pass this through?

On some other note, it does make some sense to me that these layers are a part of convolution in keras, since they are mostly used in tandem with convolutional layers on images.

[rxwei]

Can we have a separate PR for refactoring the layers into image.swift? And pass this through?

This PR introduces a purely additive change, so it would make more sense to place Cropping1D and Cropping2D directly in Image.swift in this PR.

[Shashi456]

@eaplatanios, do mention other layers you'd want to be a part of Image.swift, I just personally think that we should avoid over-granularity of Layers by separating them so much. Image.swift does make sense in this case though.

[jon-tow]

@rxwei I'll add this into Image.swift with the changes you suggested.

[rxwei]

Should the Image.swift stay in the Layers directory?

Yes. Thank you!

[eaplatanios]

@Shashi456 I haven't given this much thought other than the comment above, but there is lots of operations that are typically performed on images so I wouldn't this is granular. On the contrary, if we find that Convolutional.swift ends up being small we could potentially eventually move the convolution operations in Image.swift, as that would probably make more sense in terms of a classification hierarchy.

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

[jon-tow]

I confirm! @googlebot

[eaplatanios]

@jon-tow Could you please merge with master?

[jon-tow]

@eaplatanios Sorry for the late merge, was a bit busy.

[jon-tow]

@eaplatanios This should be good to go! Please let me know if you'd like me to add anything.

[eaplatanios]

@rxwei I started tests, but could you please handle the CLA issue?

[Shashi456]

@jon-tow if you handle the conflicts, I think this PR might be good to go as well.

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[jon-tow]

@Shashi456 Thanks for the alert! Forgot about this one :)

[saeta]

Hi @jon-tow Thank you very much for this PR, but I think that cropping-as-a-layer is relatively unused, and might be better served as a stateless function on Tensor instead. As a result, I'm going to close out this PR, but if you would like to discuss this further, let's definitely dedicate some time at an upcoming design meeting!

All the best,
-Brennan