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

Make benchmark arguments more flexible #4862

Merged
merged 5 commits into from
Jul 9, 2022

Conversation

Padarn
Copy link
Contributor

@Padarn Padarn commented Jun 25, 2022

This is currently WIP - I needed this locally to make it possible to modify these arguments in the benchmarks. If people like the suggested format I will apply this to all benchmarks.

Note because some argument are list and some are lists of lists, you would need to do

python loader/neighbor_loader.py --hetero-neighbor_sizes 5 5 --hetero-neighbor_sizes 10 10

and

python loader/neighbor_loader.py --batch-sizes 1 10 20

@codecov
Copy link

codecov bot commented Jun 25, 2022

Codecov Report

Merging #4862 (84ab29b) into master (15a6fff) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4862   +/-   ##
=======================================
  Coverage   82.71%   82.71%           
=======================================
  Files         330      330           
  Lines       17887    17887           
=======================================
  Hits        14795    14795           
  Misses       3092     3092           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15a6fff...84ab29b. Read the comment docs.

@rusty1s
Copy link
Member

rusty1s commented Jun 26, 2022

Can you clarify? Wouldn't it be easier to just pass in a list of lists? No strong opinion though :)

@Padarn
Copy link
Contributor Author

Padarn commented Jun 26, 2022

Wouldn't it be easier to just pass in a list of lists?

Hmm, maybe I misunderstood how it was meant to be used before. Using argparse I couldn't find an obvious way to pass a list of lists, except using the approach given in this MR.

@Padarn Padarn requested a review from mszarma July 3, 2022 05:48
@Padarn
Copy link
Contributor Author

Padarn commented Jul 3, 2022

Curious if anyone else has opinions on this? @mszarma you were the github suggested reviewer

@mszarma
Copy link
Contributor

mszarma commented Jul 4, 2022

Hi @Padarn and @rusty1s , using list of list with argparse is not so easy but i believe there is some way to do that, here is some POC for you: Padarn#1
Please let me know if that something that meets your needs.

@Padarn
Copy link
Contributor Author

Padarn commented Jul 4, 2022

I think your idea makes sense, let me integrate your change. Thanks!

@Padarn
Copy link
Contributor Author

Padarn commented Jul 9, 2022

Thanks for the suggestion @mszarma - have modified this PR

@Padarn Padarn force-pushed the padarn/benchmark-args branch from 17a5f88 to 84ab29b Compare July 9, 2022 09:49
@rusty1s rusty1s merged commit 5bc03a0 into pyg-team:master Jul 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants