-
Notifications
You must be signed in to change notification settings - Fork 239
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
New trainer compatibility fixes #630
New trainer compatibility fixes #630
Conversation
This PR is intended to address #629
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## ase_data_updates #630 +/- ##
====================================================
- Coverage 57.23% 57.20% -0.03%
====================================================
Files 109 109
Lines 10279 10284 +5
====================================================
Hits 5883 5883
- Misses 4396 4401 +5 ☔ View full report in Codecov by Sentry. |
More helpful warning for debug mode
LGTM, thanks @emsunshine! Let's also have @mshuaibii sign-off on this before merging. |
@emsunshine this error mentioned in #629 still occurs for me using this branch when predicting forces. I pushed a potential fix in #622, have a look please. With this and #622 the tutorial should run fine --- although these two PRs will unfortunately have minor merge conflicts once ready. |
@emsunshine can you open this PR into #622 instead. This will make the merge cleaner and I can review that once you do that. |
I did not personally encounter this error but the fix you proposed makes sense. I'm happy to merge into #622 instead. I think the conflicts will be quite minor since this PR doesn't change the ASE datasets at all. |
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.
Thanks @emsunshine! everything lgtm, the finetuning tutorial runs for me using this branch.
I just pushed a last commit directly here to avoid another merge, and added #622 in the description for provenance sakes
This PR is intended to address #629.
There were some oversights in switching to the new trainer. The following fixes are implemented:
y
toenergy
)test_ase_datasets
andtest_atoms_to_graphs
are updated with the new target namesupdate_config
function now handles configs for ASE datasets