search

aeon-toolkit / aeon

A toolkit for machine learning from time series
https://aeon-toolkit.org/
BSD 3-Clause "New" or "Revised" License
1.02k stars 128 forks source link

[ENH] Add possibility for pooling strides in TimeCNN #2307

Open flomerit opened 2 weeks ago

flomerit commented 2 weeks ago

Describe the bug

image

Steps/Code to reproduce the bug

image

Expected results

average_pooling1d_31 │ (None, 7, 6) │ 0 │ │ (AveragePooling1D) │ │

Actual results

我发现TimeCNNnet的池化操作并没有传入池化步长，导致池化步长等于avg_pool_size

Versions

No response

TonyBagnall commented 2 weeks ago

hi, I'm not that familiar, are you saying the average_pooling1d_31 layer should be (none,7,6)? @aadya940 and/or @hadifawaz1999 may be able to advise

hadifawaz1999 commented 2 weeks ago

Hello, could you clarify more on whats the issue ?

flomerit commented 2 weeks ago

hi, I'm not that familiar, are you saying the average_pooling1d_31 layer should be (none,7,6)? @aadya940 and/or @hadifawaz1999 may be able to advise Hello, could you clarify more on whats the issue ?

Yes, I feel there are issues with the outshape of the average pooling layer, according to the API description, "strides: int or list of int, default = 1.The strides of kernels in the convolution and max pooling layers, if not a list, the same strides are used for all layers. "So when strides=1, I expect the average output shape of the pooling layer =(7,6), with 7 =(8-avg_pool_size+1)//strides. So, when I went to look at the underlying code, I found the aeon \ networks \ _cnn py "conv = tf. Keras. The layers. The AveragePooling1D (pool_size = self. _avg_pool_size [i]) (conv）”The strides parameter is not passed in. So I think that the code should be changed to "conv = tf. Keras. The layers. AveragePooling1D (pool_size = self. _avg_pool_size [i], strides. = the self _strides [i]) (conv). That's probably right.

hadifawaz1999 commented 2 weeks ago

Thank you for the details. So the strides do not impact the average pooling layer as you can see in the source code but its only for the convolution layer. The reason we did that is because the original implementation did not include strides for the average pooling layer

So this is why when setting strides it wont affect the output shape of avg pooling layers because this is how the network is proposed.

TonyBagnall commented 2 weeks ago

thanks for the answer @hadifawaz1999, I dont think this is a bug, more of a feature? :)

hadifawaz1999 commented 2 weeks ago

thanks for the answer @hadifawaz1999, I dont think this is a bug, more of a feature? :)

Yes its a feature i would change the tagging to enhancement issue, happy to have this feature in, however i would prefer to separate strides form ones of conv, so keep the strides that already are in the network, and add a parameter called strides_pooling with the same setup default to None as the original paper

flomerit commented 2 weeks ago

I get it, and I'm excited to look forward to the enhanced version.

---Original--- From: "Ali El Hadi ISMAIL @.> Date: Thu, Nov 7, 2024 01:28 AM To: @.>; Cc: @.**@.>; Subject: Re: [aeon-toolkit/aeon] [ENH] Add possibility for pooling strides inTimeCNN (Issue #2307)

thanks for the answer @hadifawaz1999, I dont think this is a bug, more of a feature? :)

Yes its a feature i would change the tagging to enhancement issue, happy to have this feature in, however i would prefer to separate strides form ones of conv, so keep the strides that already are in the network, and add a parameter called strides_pooling with the same setup default to None as the original paper

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>