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

OCPCalculator output_only option #922

Merged
merged 4 commits into from
Dec 18, 2024
Merged

Conversation

misko
Copy link
Collaborator

@misko misko commented Nov 19, 2024

Some of our publicly available models have an internal config that lists output tasks that dont exist in the model. One example is "PaiNN-IS2RE-OC20-All"

We could patch each checkpoint, but instead we could also add an output_only field to our calculator, which is what this does. Additionally also adds tests for this.

@misko misko added bug Something isn't working minor Minor version release labels Nov 19, 2024
Copy link

codecov bot commented Nov 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
src/fairchem/core/common/relaxation/ase_utils.py 82.60% <100.00%> (+1.12%) ⬆️
src/fairchem/core/trainers/ocp_trainer.py 70.33% <ø> (+0.66%) ⬆️

@misko misko force-pushed the add_calculator_only_output_field branch from ff7b366 to e2c1787 Compare November 21, 2024 17:57
fix lint

undo change to packages

undo change to packages
@misko misko force-pushed the add_calculator_only_output_field branch from e2c1787 to 5e3ee75 Compare November 21, 2024 18:36
Copy link
Collaborator

@lbluque lbluque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just nits

only_output, list
), "only output must be a list of targets to output"
for key in only_output:
assert (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@@ -80,6 +80,7 @@ def __init__(
max_neighbors: int = 50,
cpu: bool = True,
seed: int | None = None,
only_output: list[str] | None = None,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, better to have only_output as a tuple, since its not meant to be changed at all in the code

@lbluque lbluque enabled auto-merge December 17, 2024 23:54
@lbluque lbluque added this pull request to the merge queue Dec 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 18, 2024
@lbluque lbluque added this pull request to the merge queue Dec 18, 2024
Merged via the queue into main with commit 83e1a53 Dec 18, 2024
8 checks passed
@lbluque lbluque deleted the add_calculator_only_output_field branch December 18, 2024 21:20
misko added a commit that referenced this pull request Jan 17, 2025
fix lint

undo change to packages

undo change to packages

Co-authored-by: Luis Barroso-Luque <[email protected]>
Former-commit-id: 3ed7a2142dee24494a8c2c0e7251a4a5ba8d3b31
beomseok-kang pushed a commit to beomseok-kang/fairchem that referenced this pull request Jan 27, 2025
)

fix lint

undo change to packages

undo change to packages

Co-authored-by: Luis Barroso-Luque <[email protected]>
Former-commit-id: 7a32302ac1427cf87dcbc2d69001d66345a2fcdb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working minor Minor version release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants