cicobalico commented 7 years ago

Hi guys, thank you all for the great job you're doing, I'm hoping this rcnn implementation will soon be available to all keras users to push the research on detection forward. Trying to test your code maybe I found a bug in the TimeDistributed implementation, at the moment it seems to have a tensor as an argument but it needs a layer for it to work as it can be seen here Doing this fixed the Exception that was being raised by keras.

so instead of:

y = keras.layers.Dense(4096)(y)  
y = keras.layers.TimeDistributed(y)

I think it needs to be:

y = keras.layers.TimeDistributed(keras.layers.Dense(4096))(y)

codecov-io commented 7 years ago

Codecov Report

Merging #38 into master will increase coverage by 0.26%. The diff coverage is 0%.

@@            Coverage Diff            @@
##           master     #38      +/-   ##
=========================================
+ Coverage   48.44%   48.7%   +0.26%     
=========================================
  Files          15      15              
  Lines         545     542       -3     
=========================================
  Hits          264     264              
+ Misses        281     278       -3

Impacted Files	Coverage Δ
keras_rcnn/models.py	`0% <0%> (ø)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f3e6f4c...d3f9c57. Read the comment docs.

JihongJu commented 7 years ago

@cicobalicoThank you for the inputs! It was indeed incorrect to apply the TimeDistributed layer to a tensor. The models.py on master is kind of buggy. This would be fixed in #27.

cicobalico commented 7 years ago

Ah, sorry for not noticing, I'll close the PR then!

broadinstitute / keras-rcnn

Fix TimeDistributed layer wrapper #38

Codecov Report