Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] fix pooling #31

Merged
merged 3 commits into from
Nov 23, 2020
Merged

[WIP] fix pooling #31

merged 3 commits into from
Nov 23, 2020

Conversation

rkurchin
Copy link
Member

Goals:

  • fix edge case issue of calculating pool parameters where pad can end up larger than dim and give guaranteed zeros at the edges (was causing NaN errors in training, plus just doesn't make sense)
  • merge max and mean pool into one pool type to avoid duplicate code
  • new type will precalculate and store pool parameters to speed up forward pass

Currently, having some issues with the that last bullet point involving the PoolDims object which I'm corresponding with @DhairyaLGandhi on Slack about.

Still to do:

  • fix aforementioned issue
  • finish updating tests to use new type

@paulxshen, tagged you as reviewer. I know you're probably still familiarizing yourself with the codebase, so take your time, please ask for any clarification you need, and no need to look at this until Dhairya and I sort out the issue I'm having. Meantime, I'll keep working on docs :)

@rkurchin rkurchin added the bug Something isn't working label Nov 20, 2020
@rkurchin rkurchin self-assigned this Nov 20, 2020
@DhairyaLGandhi
Copy link
Member

Is amortizing the cost of computing the pooldims significant?

@rkurchin
Copy link
Member Author

rkurchin commented Nov 20, 2020

To be honest, I'm not sure, I haven't done a detailed set of timings, but it seemed like it should be a straightforward thing to do and would certainly save time, even if I don't know exactly how much. If it's complicated in a way that I'm not seeing, I can obviously undo it easily, it just seemed like it should probably be an easy fix...is there something I'm missing here?

@paulxshen
Copy link

Got it Rachel - will look over. Thanks!

@codecov-io
Copy link

Codecov Report

Merging #31 (a0b2421) into master (163d2a3) will decrease coverage by 2.45%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #31      +/-   ##
==========================================
- Coverage   75.00%   72.54%   -2.46%     
==========================================
  Files           3        3              
  Lines          56       51       -5     
==========================================
- Hits           42       37       -5     
  Misses         14       14              
Impacted Files Coverage Δ
src/AtomicGraphNets.jl 100.00% <ø> (ø)
src/layers.jl 71.42% <84.61%> (-4.58%) ⬇️
src/models.jl 100.00% <100.00%> (+40.00%) ⬆️

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 163d2a3...a0b2421. Read the comment docs.

@rkurchin rkurchin merged commit 8d52bd0 into master Nov 23, 2020
@rkurchin rkurchin deleted the fix_pooling branch November 23, 2020 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants