update shortest_path to TF2 #119

maguileracanon · 2020-05-29T16:57:54Z

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 · 2020-05-29T16:57:57Z

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).

📝 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 · 2020-05-29T17:00:19Z

@googlebot I signed it!

googlebot · 2020-05-29T17:00:22Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

alvarosg · 2020-06-02T17:21:04Z

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 · 2020-06-03T12:36:39Z

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 · 2020-06-03T13:22:46Z

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 · 2020-06-03T14:48:08Z

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 e82bd5b 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 · 2020-06-03T15:13:45Z

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).


        delete output (+4 squashed commit)

Squashed commit: [e47daf1] Revert "update to tf2" This reverts commit a292023. [a292023] update to tf2 [9c55e7d] copy to tf2 folder [059d4bb] update shortest_path to TF2


        update to tf2 with diff working

maguileracanon · 2020-06-11T17:25:34Z

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

alvarosg

Thanks for your PR great work! Please fine below some comments.

I think we should actually be able to achieve the same with quite a few line changes, to keep it closer to the tf1 version, and to the other tf2 version.

Something else that I noticed, is that when I run this demo agains the old tf2 version in colaboratory
I got quite different learning curves in the middle plot:

Tf2 version:

Tf1 version:

It may be worth investigating, although maybe it is just different default initialization settings or something like that.

alvarosg · 2020-06-23T10:57:09Z