Skip to content

Conversation

@carusyte
Copy link
Contributor

@carusyte carusyte commented Aug 6, 2018

This PR is intended to be (hopefully) a better organized one to #35 ...
Recap:

  1. Pip installable: One can generate installable packages by running python setup.py sdist under the root directory of this repo and then install it by pip install ./dist/dnc-0.0.2.tar.gz. Some versioning and author information may not be correct, which can be edited or enriched in the setup.py file.
  2. tf.unstack can't process tensors with partially-known shape info. Methods in the util.py are refactored by utilizing a combination of tensor operations as a workaround, meanwhile ensuring those operations can be run in GPU.
  3. Performance. Some operations (or its gradients) can't be run in GPU as of tensorflow version 1.9(e.g. tf.reduce_prod, see #3957,#8841, and tf.nn.top_k, see #5719).
  4. Replace deprecated APIs. Some methods/function arguments are deprecated as of tf v1.9

Copy link
Contributor

@dm-jrae dm-jrae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for addressing the issues - it's looking great, no worries re. the pull request.

Just want to check on the train script though, it looks like there's no dnc import right now and it should fail. Can you just double check this (haven't unit tested the training script).

import tensorflow as tf
import sonnet as snt

import dnc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want this import in, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I just missed this one. I'm short of budgets and all machines(only 2 actually) at my disposal are busy running something, so I didn't bother to run the script...

Now it seems able to start and run smoothly, but I don't have the computer power to run it through.

If there's any other issues, please don't hesitate to point them out.

Copy link
Contributor

@dm-jrae dm-jrae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks for the fix :-)

@dm-jrae dm-jrae merged commit 22deec1 into google-deepmind:master Aug 6, 2018
@carusyte carusyte deleted the carusyte_fix branch August 6, 2018 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants