-
Notifications
You must be signed in to change notification settings - Fork 7
Seven net and nequix configs #169
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
Conversation
vbharadwaj-bk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine.. but do the benchmark replication commands here work https://passionlab.github.io/OpenEquivariance/tests_and_benchmarks/#replicating-our-benchmarks? Doesn't have to look the same since competing package versions have advanced, but plots shouldn't be badly formatted / out of whack.
https://passionlab.github.io/OpenEquivariance/tests_and_benchmarks/#replicating-our-benchmarks
|
This is a good point. I was thinking about this because I think the specialization of the benchmarking framework towards recreating the presentation of the results is a little problematic as is. I didn't get into it in this PR, but I could create another which addresses it and we could merge that first, and only merge this after. For example, when I was performing the benchmarking for these configs, I could not use cuEquivariance because it does not support configs with irreps like 64x1o + 128x1e, which are used for these configs, so then I have to remove it from the tested implementations, but then, the plotting function explicitly looks to normalize against cueq... so the plotting breaks. And it's not just these new configs, the plotting doesn't work out of the box in many situations where the benchmarking parameters differ from the defaults. My proposal would be to make the plotting functions that get called by the benchmarking equipment more general purpose, so they always work, then break out the "reproduce paper plots" functionality into some scripts called like, "paper_roofline_fig.py" which are specific calls to the general purpose benchmarking machinery, and then these scripts contain and isolate the specific plotting instructions like normalizing against a certain implementations, expecting certain names, expecting certain datatypes. What do you think of this proposal? |
|
Making the plotting general is okay - but if you want to merge this quickly, I would suggest just disabling the additional problems conditionally when the flag --plot is set. That's only a few lines and will keep the existing functionality stable. We don't really expect to generate plots again and are keeping that code around purely for reproducibility / good science -- on the other hand we have gotten quite a few requests for JAX support.. |
|
Maybe I should just add the configs to the repo but not make any changes to the benchmarking file at all. The most minimal change. |
add seven net and nequix configs to the repo and benchmark suite