update shortest_path to TF2

maguileracanon commented 4 years ago

Hello, graph-nets Authors!! Following the discussion in #115 I wanted to contribute and adapted the shortest_path example to run in TF2 and implementing the sonnet Adam optimizer. I added a couple of functions to make it runnable. Ideally, they should probably live inside utils_np or utils_tf but I kept them here for simplicity. I hope it is useful!

googlebot commented 4 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

maguileracanon commented 4 years ago

@googlebot I signed it!

googlebot commented 4 years ago

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

alvarosg commented 4 years ago

Thanks for the PR and the initiative @maguileracanon !

To make the review easier, do you think it would be possible to:

Empty the notebook outputs (we prefer not committing binary data, and the demos are super easy to run in Google Colaboratory anyway).
Because this forks an existing TF1 file, ideally, the first commit would be a forked version of the tf1 version verbatim, and the second commit should show a clean diff for reviewing purposes of the new code, do you think that you can split the commits into that?

Thanks in advance :)

alvarosg commented 4 years ago

Thanks, do you think it would be possible to rewrite the commit history to have just those two CLs (Similarly to how it was done here ).

Currently, the diff of the "update to tf2" does not pick up on the specific changes that were made (ideally the changes would be able to highlight the tf2 difference specifically). If possible it would look more similar to this diff.

I know github support for diffs in colab notebooks is not great, and maybe the changes are large enough that the diff cannot figure out the shared parts. But otherwise, it is hard to see what update to tf2 and Revert "update to tf2"commits are doing :)

maguileracanon commented 4 years ago

Hi ! yes. Sorry, the revert commit was because, as you mention, I noticed that the diff for the first 2 commits wasn't picking up the difference between them so I tried to revert it and see if I can find another solution. I still don't really understand why is taking the diff as if I had deleted everything and placed it again. I'll post a comment when I solve it.

maguileracanon commented 4 years ago

Ok, the problem was that I was deleting the output at the end and that was kind of re-writting the entire notebook. If you look at the last commit e82bd5befac1847f68bf7aa4fa00e74a28215af4 then the diff should be clear for review. I hope this is what you need, but let me know if there is something else you need.

alvarosg commented 4 years ago

That looks much better! It could be great if you could get rid of the earlier commits so it shows a single first commit with the full copy (e.g. a large diff), and then the second commit with a diff similar to what you have now in the last commit. Otherwise, it is hard to track all of the changes in the earlier commits.

One option could be to squash all of the commits (except the last one). This should result in a single commit that corresponds purely to the file move.

Alternatively, it may be easier to start again, by:

squash everything and moving the resulting commit into a different branch.
remove all commits from this branch
creating a single new commit in this branch just copying the demo file into the tf2 folder.
rebase the commit you cherrypicked back to this branch (if everything else was correct, this should also give you a small diff after the rebase).

maguileracanon commented 4 years ago

ok. The commit history looks a little bit tidier now :)

alvarosg commented 3 years ago

Hi @maguileracanon , I sent my review a while back, please let me know if you are still planning to get back to this :)

google-deepmind / graph_nets