ASE Calculator Updates #299
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… expected by certain models, removing keys that are added by concatenate_keys.
laserkelvin
requested changes
Oct 1, 2024
matsciml/interfaces/ase/base.py
Outdated
| # run the data structure through the model | ||
| output = self.task_module.predict(data_dict) | ||
| else: | ||
| output = self.task_module.predict(atoms) |
There was a problem hiding this comment.
So this requires other models to have a predict method as well?
If it's a model that isn't a matsciml model, I don't know if it's a better idea to just simply run their __call__ or forward method. If you don't think that's a good idea or have a reason for doing so, you should add in an assertion in the __init__ that checks that the input model has a predict function.
There was a problem hiding this comment.
Yeah, actually I think just running their forward method would be good. Updated here 81f1ab0
…ove unsued code in tests
laserkelvin
approved these changes
Oct 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pr updates the ASE calculator interface to accommodate a few changes:
ForceRegressionTask, etc.forcegets mapped intoforces. This mapping should now be passes as a dictionary to the calculator on instantiation if required._calculateto build the results dictionary.concatenate_keysfunction which also adds properties to PyG graphs that are expected by models like MACE, add a few extra keys to_format_atoms, and change when type casting is called.matglmodel.