-
Notifications
You must be signed in to change notification settings - Fork 138
Adding GlobalMaxPooling 1D, 2D, 3D #76
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Could you please add some tests to Tests/DeepLearningTests/LayerTests.swift
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice. Thank you! As @dan-zheng said, tests should be added.
@dan-zheng I'm working on the test cases now, I will have a seperate PR for them, there are a lot of layers which do not have test cases and I will make a list of the same in #77, it would be easier to track them that way. |
@rxwei can this be merged? |
Could you pull from master? I wanted to trigger a rebuild. |
@rxwei the docker summary fails because it says the max function is not differentiable, is there any way correct that? |
We could either add a VJP for the max op to the standard library, or just use the actual "max pooling" operation inside the global max pooling layers (set the pool size to the width & height of the input). |
@tanmayb123 I was trying to do that when i initially added the max function to the batch normalized vjp, i just put it in the wrong place. I think we should rather put in the max function because it is actually used in multiple places, might come in as a handy operation. |
@rxwei could you tell me how i could go about defining a vjp for the max function? |
I'm sorry for the delayed response! Recently the team has been busy with releasing v0.3 and getting some important core pieces like RNNs into the library. I'll definitely help you with defining derivatives for |
In the meantime, it would be useful to explore our custom differentiation tutorial, and see how other derivatives are defined, e.g. |
Adding GlobalMaxPooling 1D, 2D, 3D
@rxwei do you mind triggering a build on this? Thanks to @eaplatanios for adding the max and min vjps. With this, we are done with all the pooling layers. |
@rxwei could you take a quick glance and trigger the build once more? This does pass locally now. Also is there any way to keep updating the toolchain directly, instead of continuously downloading nightlies xD. |
You can always build a toolchain from the tensorflow branch on apple/swift using the |
@eaplatanios sorry to bother you with this. But could you tell me if the max function call i wrote for this is wrong? The error that pops is |
@Shashi456 Could you post the full error trace? I can clone and take a look in a bit. |
|
I see. The |
Done in #159. |
#159 has been merged. Could you do a pull? |
@rxwei done. pretty happy that finally done with pooling layers xD |
@rxwei I'll take care of both the PRs, in a while. Sorry! |
@rxwei built and tested with nightly. This PR passes as is. The build now takes substantially more time though somehow xD. I'm not sure about the test since the linux error you've mentioned popped up, but i rectified the error you mentioned about Edit: Tested this by commenting out the Linux errors, The test added by this PR pass. I think this is good to go. |
@rxwei could you trigger a build on this? |
@rxwei The errors aren't from these additions. I think this can be merged, |
@rxwei I think this is ready to be merged. |
I've added the max operation to operators, since it wasnt defined earlier.