Improve additive models #427
Open
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
frostedoyster
force-pushed
the
improve-additive
branch
from
024e4f5 to
9277267
Compare
frostedoyster
force-pushed
the
improve-additive
branch
from
53197a0 to
aded37d
Compare
abmazitov
reviewed
Dec 28, 2024
| @@ -122,7 +122,7 @@ def test_regression_train(): | |||
| ) | |||
|
|
|||
| # if you need to change the hardcoded values: | |||
| torch.set_printoptions(precision=12) | |||
| print(output["mtt::U0"].block().values) | |||
| # torch.set_printoptions(precision=12) | |||
There was a problem hiding this comment.
This is a leftover I suppose?
abmazitov
reviewed
Dec 28, 2024
| fixed_weights: Optional[Dict[str, Dict[int, str]]] = None, | ||
| ) -> None: | ||
| """Train/fit the composition weights for the datasets. | ||
|
|
||
| :param datasets: Dataset(s) to calculate the composition weights for. | ||
| :param fixed_weights: Optional fixed weights to use for the composition model, | ||
| for one or more target quantities. | ||
| :param additive_models: Additive models to be removed from the targets |
There was a problem hiding this comment.
Why this removal should happen implicitly in the composition model class? Maybe it is better to have a separate utility function that removes additives?
abmazitov
reviewed
Dec 28, 2024
| systems, targets = systems_and_targets_to_device( | ||
| systems, targets, device | ||
| ) | ||
| for additive_model in additive_models: |
There was a problem hiding this comment.
I mean, I would move this for-loop out of the composition model class, it feels like this functionality goes out of the scope of the CompositionModel
abmazitov
reviewed
Dec 28, 2024
| f"Composition model does not support target quantity " | ||
| f"{target_info.quantity}. This is an architecture bug. " | ||
| f"Composition model does not support target " | ||
| f"{target_name}. This is an architecture bug. " |
There was a problem hiding this comment.
Why the lack of support of the target is an architecture bug? If would not call it a bug, in fact, but maybe it’s just a copy-paste glitch
abmazitov
reviewed
Dec 28, 2024
| "ZBL only supports eV units, but a " | ||
| f"{target.unit} output was provided." | ||
| f"ZBL model does not support target " | ||
| f"{target_name}. This is an architecture bug. " |
abmazitov
reviewed
Dec 28, 2024
| if not self.is_valid_target(target_name, target_info): | ||
| raise ValueError( | ||
| f"ZBL model does not support target " | ||
| f"{target_name}. This is an architecture bug. " |
abmazitov
reviewed
Dec 28, 2024
| @@ -261,6 +268,38 @@ def requested_neighbor_lists(self) -> List[NeighborListOptions]: | |||
| ) | |||
| ] | |||
|
|
|||
| @staticmethod | |||
| def is_valid_target(target_name: str, target_info: TargetInfo) -> bool: | |||
| """Finds if a ``TargetInfo`` object is compatible with a composition model. | |||
There was a problem hiding this comment.
Compatible with ZBL model I guess?
|
The rest of the code looks good to me, my main concern is the CompositionModel |
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.
Improves the additive models and closes #355. In particular:
Contributor (creator of pull-request) checklist
Documentation updated (for new features)?📚 Documentation preview 📚: https://metatrain--427.org.readthedocs.build/en/427/