-
Notifications
You must be signed in to change notification settings - Fork 7
Chore: allow instantiation of calc based on task #372
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
base: main
Are you sure you want to change the base?
Chore: allow instantiation of calc based on task #372
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.
Pull Request Overview
This PR refactors model instantiation to support multi-domain models (molecules vs. materials) by deferring model creation until the task domain is known. The key changes enable models like those trained on OMol25 to use different calculator heads based on whether they're processing molecular or materials tasks.
- Splits model gathering into parameter collection (
gather_model_params) and late model instantiation (gather_model) - Adds
model_domainandsupports_omolfields to support domain-specific calculator initialization - Changes
calcfrom a cached property to a regular property with setter support for testing
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| lambench/workflow/entrypoint.py | Refactors model gathering to defer instantiation; adds domain-based model creation |
| lambench/models/basemodel.py | Adds supports_omol and model_domain attributes for multi-domain support |
| lambench/models/ase_models.py | Changes calc to property with setter; adds domain-aware calculator initialization |
| lambench/metrics/utils.py | Updates to use new gather_model_params and gather_model functions |
| tests/workflow/test_entrypoint.py | Updates test fixture to return dict instead of DPModel instance |
| tests/tasks/calculator/test_nve_md.py | Simplifies test fixtures by leveraging default EMT calculator fallback |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import pytest | ||
| from ase import Atoms | ||
| from ase.calculators.emt import EMT | ||
| from lambench.models.ase_models import ASEModel |
Copilot
AI
Oct 29, 2025
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.
Import of 'Calculator' is not used.
| from lambench.models.ase_models import ASEModel |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #372 +/- ##
==========================================
+ Coverage 65.89% 65.96% +0.06%
==========================================
Files 35 35
Lines 1589 1604 +15
Branches 189 192 +3
==========================================
+ Hits 1047 1058 +11
- Misses 500 502 +2
- Partials 42 44 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
lambench/models/ase_models.py:1
- The property getter doesn't check if
self._calcis already initialized before recreating the calculator. This means every access tocalcwill reinitialize the calculator, which is inefficient and may cause issues. Add a check:if self._calc is not None: return self._calcat the beginning of the property getter.
from __future__ import annotations
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This PR refactors the logic of calculator instantiation. We anticipate more LAMs to be trained with OMol25, MPTjr, and Omat24 together in the near future. As a consequence, it makes more sense to use domain-match modality for some generalizability tests. In an upcoming PR, we will introduce three OOD-FF test sets with two different DFT settings matching OMol25 and MPtrj, respectively.