-
Notifications
You must be signed in to change notification settings - Fork 15
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
Release single-step evaluation framework and wrappers for several model types #14
Conversation
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.
I left a few small comments, but overall looks good to me. The tests all pass locally for me.
I was not able to run eval.py
locally because I don't have the correct model files or example configs, so I can't independently confirm that it works. By extension, I could not test the "Make the evaluation pipeline easy to extend" commit. Did you test it locally @kmaziarz ? If so I am happy to merge it, and provide documentation / configs / model weights in a future PR.
Yes, I re-ran the single-step evaluation for all seven model types, and confirmed that all resulting numbers match the table in the paper 🙂 |
11c81ce
to
c06669c
Compare
This (fairly large) PR releases the majority of the single-step evaluation code, including the eval script itself, inference wrappers for seven model types, and glue code to plug these models into search. The changes are split into semantically meaningful commits. Some notes for reviewing:
mypy
to ignore external libraries it doesn't know about.syntheseus
from outside.A few notable things which are still missing, to be added in future PRs to avoid making this one even larger:
syntheseus/reaction_prediction
(currently the coverage is quite low, partly because the wrappers themselves cannot be tested easily without setting up dedicated environments for each, but there are also places which are just not adequately covered by tests yet).