Skip to content
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

add ema to BaseTrainer init #916

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

IliasChair
Copy link
Contributor

This is a follow-up to PR #909

The original changes were made directly on the main branch of my fork. To continue working on my fork, it was necessary to move these changes to a dedicated feature branch and resubmitting them through this PR. The code changes remain the same.

Apologies for the mess!

Running user-generated models with the OCPCalculator currently raises an error:
'OCPTrainer' object has no attribute 'ema' at line 621 in the load_checkpoint function of base_trainer.py.

This issue occurs because self.ema is not declared when load_checkpoint is called. The ema attribute is currently declared in load_extras rather than in the init method of BaseTrainer, leading to this error if load_extras is not called beforehand. As a best practice, class attributes should only be declared within init.

This PR addresses the issue by adding ema initialization to the init function of the BaseTrainer class and includes a check to ensure ema is not None when loading the state dict in load_checkpoint.

This fix resolves the error in my testing.

@misko misko added bug Something isn't working minor Minor version release patch Patch version release and removed minor Minor version release labels Nov 19, 2024
@misko misko added this pull request to the merge queue Nov 21, 2024
Merged via the queue into FAIR-Chem:main with commit 6a634ab Nov 21, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working patch Patch version release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants