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

Merging configs #362

Merged
merged 64 commits into from
Jun 23, 2023
Merged

Merging configs #362

merged 64 commits into from
Jun 23, 2023

Conversation

DomInvivo
Copy link
Collaborator

Changelogs

  • Changed the main config files to "override" the gcn configs, such that we avoid copy-pasting full configurations.
  • Re-organized the loading of the yaml file into functions

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

hadim and others added 30 commits June 7, 2023 07:05
Copy link
Contributor

@zhiyil1230 zhiyil1230 left a comment

Choose a reason for hiding this comment

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

ah nice, looks good to me.

@DomInvivo
Copy link
Collaborator Author

@zhiyil-graphcore , before I merge, can you double-check that it works well?

if args.config is not None:
CONFIG_FILE = args.config
args, unknown_args = parser.parse_known_args()
cfg = load_yaml_config(CONFIG_FILE, MAIN_DIR, unknown_args)

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 it would be more convenient to add these two lines so that we could load config from command line:

`    if args.config is not None:
        CONFIG_FILE = args.config`

@zhiyil1230
Copy link
Contributor

@zhiyil-graphcore , before I merge, can you double-check that it works well?

I tested the large mix configs, they run ok with merged dicts.

@@ -172,7 +185,7 @@ architecture:

gnn: # Set as null to avoid a post-nn network
in_dim: 64 # or otherwise the correct value
out_dim: &gnn_dim 128
out_dim: &gnn_dim 64
Copy link
Contributor

Choose a reason for hiding this comment

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

We might need a out_dim of 128 for gcn so that the model size for the 3 models (gcn, gin, gine) stay around 170k. It's not a big deal if this is more of a sample script as we still have those baseline script to reproduce the results reported on paper.

@zhiyil1230 zhiyil1230 mentioned this pull request Jun 23, 2023
6 tasks
@zhiyil1230 zhiyil1230 merged commit 64a1fc3 into main Jun 23, 2023
@DomInvivo DomInvivo deleted the merging_configs branch July 21, 2023 17:49
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.

5 participants