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

Unintuitive --normalize_features option in benchmark scripts #4966

Closed
gau-nernst opened this issue Jul 12, 2022 · 3 comments · Fixed by #4967
Closed

Unintuitive --normalize_features option in benchmark scripts #4966

gau-nernst opened this issue Jul 12, 2022 · 3 comments · Fixed by #4967
Labels

Comments

@gau-nernst
Copy link
Contributor

🐛 Describe the bug

In the reference benchmark script for GAT here, the --normalize-features option is probably not behaving what it is intended to do. As noted by the official Python argparse documentation

The bool() function is not recommended as a type converter. All it does is convert empty strings to False and non-empty strings to True. This is usually not what is desired.

In other words,

python gat.py --dataset=Cora --normalize_features=False  # this will silently evaluate to normalize_features=True
python gat.py --dataset=Cora --normalize_features=0  # same problem as above, since "0" is a string
python gat.py --dataset=Cora --normalize_features="" # the only way to turn off feature normalization correctly

One alternative would be

parser.add_argument('--no-normalize_features', action="store_true")
...

dataset = get_planetoid_dataset(args.dataset, not args.no_normalize_features)

It is not a huge bug, but I think it would improve usability for the users, as well as providing better reference scripts.

Environment

  • PyG version: 2.0.4
  • PyTorch version: 1.11.0
  • OS: macOS (ARM)
  • Python version: 3.10
  • CUDA/cuDNN version: NA
  • How you installed PyTorch and PyG (conda, pip, source): PyTorch through conda, PyG through pip
  • Any other relevant information (e.g., version of torch-scatter): NA
@gau-nernst gau-nernst added the bug label Jul 12, 2022
@rusty1s rusty1s linked a pull request Jul 12, 2022 that will close this issue
@rusty1s
Copy link
Member

rusty1s commented Jul 12, 2022

Thanks! It is fixed in #4967.

@gau-nernst
Copy link
Contributor Author

Thanks for the speedy fix, and also fixing other bool arguments!

I notice that you miss out not for GCN and SGC scripts

dataset = get_planetoid_dataset(args.dataset, args.no_normalize_features)

dataset = get_planetoid_dataset(args.dataset, args.no_normalize_features)

The rest are good!

@rusty1s
Copy link
Member

rusty1s commented Jul 12, 2022

Oh shit, I was too fast. Thanks for checking!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants