Skip to content

ChemPropModel.aggregation is ignored when from_foundation is set, and "attentive" isn't a valid value #587

Description

@smcolby

Summary

ChemPropModel.build() hardcodes MeanAggregation whenever from_foundation is set, silently ignoring the configured aggregation field. Separately, validate_aggregation only accepts "mean" or "norm", even though chemprop.nn also provides AttentiveAggregation.

Where

openadmet/models/architecture/chemprop.py:

  • build(), from_foundation branch (~line 649): always constructs aggr = nn.MeanAggregation(), regardless of self.aggregation.
  • build(), else branch (~lines 657-660): this is the only place self.aggregation is actually consulted, and only reachable when from_foundation is falsy.
  • validate_aggregation (~lines 524-542): raises unless value is "mean" or "norm"; chemprop.nn.AttentiveAggregation exists but is unreachable through this field.

Why this is a bug, not intentional behavior

Aggregation pools the message-passing body's already-computed per-atom hidden states into a molecule-level embedding. MeanAggregation/NormAggregation have no learned parameters, and AttentiveAggregation adds a small learned layer on top of the aggregation step, independent of the message-passing body's own weights. There's no architectural reason a foundation-initialized encoder (CheMeleon or a custom checkpoint) can't use a different aggregation strategy — the current hardcoding looks like it was never revisited after from_foundation was added, not a deliberate constraint.

The logger.warning at that call site ("Using a foundation model overrides settings for depth, message_hidden_dim, messages, and aggregation") documents the current behavior accurately, but it means the aggregation recipe field is a silent no-op for anyone using from_foundation.

Suggested fix

  • In the from_foundation branch, build aggr from self.aggregation the same way the else branch does, instead of hardcoding MeanAggregation.
  • Extend validate_aggregation to accept "attentive", and extend the aggregation-class dispatch to include nn.AttentiveAggregation.
  • Update the warning message to drop "and aggregation" once it's no longer overridden.

Repro

Any anvil recipe with model.params.from_foundation: chemeleon and model.params.aggregation: norm (or any non-"mean" value) trains with mean aggregation regardless, with no error or additional warning tied to the ignored value.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions