Skip to content

Add Spin/Charge System Conditioning to PET#1040

Open
JonathanSchmidt1 wants to merge 9 commits intometatensor:mainfrom
JonathanSchmidt1:only_system_conditioning
Open

Add Spin/Charge System Conditioning to PET#1040
JonathanSchmidt1 wants to merge 9 commits intometatensor:mainfrom
JonathanSchmidt1:only_system_conditioning

Conversation

@JonathanSchmidt1
Copy link

@JonathanSchmidt1 JonathanSchmidt1 commented Feb 9, 2026

Add charge/spin system conditioning module that has extra embeddings for charge and spin that are added to node embeddings at each message passing step
Add respective hyperparameters and dataset options

Contributor (creator of pull-request) checklist

  • Tests updated (for new features and bugfixes)?
  • Documentation updated (for new features)?
  • Issue referenced (for PRs that solve an issue)?

Maintainer/Reviewer checklist

  • CHANGELOG updated with public API or any other important changes?
  • GPU tests passed (maintainer comment: "cscs-ci run")?

📚 Documentation preview 📚: https://metatrain--1040.org.readthedocs.build/en/1040/

@JonathanSchmidt1
Copy link
Author

Ps: Sorry about the undescriptive name seems like I cant change it or I am too stupid ^^

if "charge" in system_options:
charge_key = system_options["charge"]["key"]
self.charge_array = MemmapArray(
path / f"{charge_key}.bin", (self.ns,), "float32", mode="r"
Copy link
Member

Choose a reason for hiding this comment

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

Do we already have .bin files in the dataset?

Copy link
Author

Choose a reason for hiding this comment

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

I think so

Copy link
Member

Choose a reason for hiding this comment

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

ok. Then this would mainly need some documentation in the user doc, saying that we'll check for data at this key and put it in the system under this name

@Luthaf Luthaf changed the title Only system conditioning Add Spin/Charge System Conditioning to PET Feb 9, 2026
@Luthaf
Copy link
Member

Luthaf commented Feb 9, 2026

cscs-ci run


class Classifier(ModelInterface[ModelHypers]):
__checkpoint_version__ = 1
__checkpoint_version__ = 2
Copy link
Contributor

Choose a reason for hiding this comment

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

@frostedoyster is this needed? From the last ml-devel meeting I understood that in principle changes to an architecture's checkpoint shouldn't need a version upgrade on the wrapper.

I think what needs to be done is to simply upgrade the checkpoint file with the new file, without any renaming, but let's see what Filippo says.

(same for llpr)

@pfebrer
Copy link
Contributor

pfebrer commented Feb 10, 2026

Also, I think passing global data in general is something very useful. It would be nice if the user could pass whatever global data and it would be automatically embedded into PET. Could we not hardcode it to spin and charge? We could still allow the user to do e.g. type: spin if spin needs special treatment.

I'm thinking that the input could look something like:

systems:
    read_from: ...
    ...
    features:
        - key: spin
          per_atom: false # Maybe not needed and can be inferred?
          type: spin # So that min-max gets automatically assigned (?)

Or maybe features would be a separate yaml section from systems, idk

@JonathanSchmidt1
Copy link
Author

Discussed a bit with @pfebrer . We could generalize it but we have to take into account that the implementation only supports integer embeddings right now and we might also want to take the embedding specs already existing in MACE into account.

@pfebrer
Copy link
Contributor

pfebrer commented Feb 12, 2026

Yes, regarding this

and we might also want to take the embedding specs already existing in MACE into account.

the thing is that as they are right now, charge and spin are inputs in the base hyperparameters. So if we don't generalize, we would at least need to make sure that these inputs also make sense for other architectures. Otherwise we could make them temporarily inputs of the PET architecture until we find a good solution, but I'm not sure this is even possible (?).

I think the issue is that this situation requires something that had never happened in metatrain: we need to use additional inputs (not just coordinates and atomic numbers). Maybe extra_data can be used for this? 🤔 In any case, probably worth a discussion since this case will establish a precedent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants