-
Notifications
You must be signed in to change notification settings - Fork 3
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
implemented apax batch evaluation node #280
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.
Looks good, primarily concerned with the ipsuite
import
apax/nodes/analysis.py
Outdated
from ipsuite import fields | ||
|
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.
if you import IPSuite here, you could use the base class as well.
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.
fair, I'll switch to the base class
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.
ah yeah this is also why the tests fail, my bad
apax/nodes/__init__.py
Outdated
from .analysis import ApaxBatchPrediction | ||
from .md import ApaxJaxMD | ||
from .model import Apax, ApaxEnsemble | ||
|
||
__all__ = ["Apax", "ApaxEnsemble", "ApaxJaxMD"] | ||
__all__ = ["Apax", "ApaxEnsemble", "ApaxJaxMD", "ApaxBatchPrediction"] |
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.
Shall we split this into Nodes that work out-of-the-box and models that depend on IPSuite.
With a
with contextlib.supress(ImportError)
from .analysis import ApaxBatchPrediction
__all__.append("ApaxBatchPrediction")
This would also allow the BatchSelection until ipsuite-core
is ready.
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.
sounds good
I wrapped the import in a try except block and fixed the module path. |
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.
👍
batch evaluation node suitable for evaluating many differently sized structures. Batch size of 1 is sufficient.