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

Fixed GatedGCN in float16. Minor cleanups. #426

Merged
merged 3 commits into from
Aug 3, 2023
Merged

Fixed GatedGCN in float16. Minor cleanups. #426

merged 3 commits into from
Aug 3, 2023

Conversation

DomInvivo
Copy link
Collaborator

Changelogs

  • Added an eps parameter to the GatedGCNPyg layer, with a default value of 1e-3 since 1e-6 was causing numerical issues in float16.
  • Removed some useless lines of code from the FullGraphMultiTaskNetwork.forward

Checklist:

  • Was this PR discussed in an issue? It is recommended to first discuss a new feature into a GitHub issue before opening a PR.
  • Add tests to cover the fixed bug(s) or the new introduced feature(s) (if appropriate).
  • Update the API documentation is a new function is added, or an existing one is deleted.
  • Write concise and explanatory changelogs above.
  • If possible, assign one of the following labels to the PR: feature, fix or test (or ask a maintainer to do it for you).

discussion related to that PR

@DomInvivo DomInvivo added the fix PR that fixes an issue label Aug 1, 2023
@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #426 (c7966f0) into main (57399b3) will decrease coverage by 0.03%.
Report is 14 commits behind head on main.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##             main     #426      +/-   ##
==========================================
- Coverage   66.91%   66.88%   -0.03%     
==========================================
  Files          82       82              
  Lines        7819     7821       +2     
==========================================
- Hits         5232     5231       -1     
- Misses       2587     2590       +3     
Flag Coverage Δ
unittests 66.88% <72.72%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
ipu 49.14% <ø> (ø)

@notion-workspace
Copy link

Fix the GatedGCN model

Comment on lines -1158 to -1163
g["feat"] = g["feat"]
e = None

if "edge_feat" in get_keys(g):
g["edge_feat"] = g["edge_feat"]

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand why this was in the code in the first place?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hahaha, My guess is that, when killing DGL in favor for PyG only, we did a ctrl+F to replace all calls to g.get_node_feats("feat") by g["feat"]

@callumm-graphcore
Copy link
Collaborator

GatedGCN part looks fine to me - have you tried to train in 16-true with this change?

@DomInvivo
Copy link
Collaborator Author

GatedGCN part looks fine to me - have you tried to train in 16-true with this change?

I don't have access to an IPU to train with 16-true. I can only do mixed on GPU

@DomInvivo DomInvivo merged commit d7c910e into main Aug 3, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix PR that fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants