Skip to content

Introduce more extensible scheme for extra properties.#464

Draft
CompRhys wants to merge 11 commits intomainfrom
extensible-extras
Draft

Introduce more extensible scheme for extra properties.#464
CompRhys wants to merge 11 commits intomainfrom
extensible-extras

Conversation

@CompRhys
Copy link
Member

This is a rough draft to try unlock #463

@CompRhys CompRhys added the api API design discussions label Feb 23, 2026
spin: torch.Tensor | None = field(default=None)
system_idx: torch.Tensor | None = field(default=None)
_constraints: list["Constraint"] = field(default_factory=lambda: []) # noqa: PIE807
_system_extras: dict[str, torch.Tensor] = field(default_factory=dict)
Copy link
Member Author

Choose a reason for hiding this comment

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

could consider removing privileged positions of spin and charge

static_state.store_model_extras(model_outputs)

props = trajectory_reporter.report(static_state, 0, model=model)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would probably be more in favor of getting rid of trajectory_reporter here and just concatenate the results returned. We should just copy the logic of detecting whether the property is per atom or per system.

Trajectory_reporter have more functionalities than that but I'm not sure many people use it when running this static() function

@thomasloux
Copy link
Collaborator

Nice work!

I think that most of the time extra predicted properties are not useful in MD, so that _system_extras and _atom_extras are not useful most of the time, but why not for analysis purposes. It's mainly the extra inputs that are important. More specifically, as torch_sim is transitioning to externally defined Model, they should be a very clear and canonical way to add extra_input in State that can be used easily but the models.

@CompRhys
Copy link
Member Author

More specifically, as torch_sim is transitioning to externally defined Model, they should be a very clear and canonical way to add extra_input in State that can be used easily but the models.

I think here the aim was to just get something that would be general enough to be both input and output similar to info on ASE atoms. The prompting here was primarily focussed on hitting the generality point whilst trying to have a reasonable interface. The main questionable thing about this approach is that accessing the "extras" via the property syntax simstate.external_E_field isn't currently typed.

Is this sufficient for the matris use-case? do you want to try explore that on top of this and then we can revisit this design?

@Asecretboy would you be interested in adding a torchsim interface to https://github.com/HPC-AI-Team/MatRIS as an alternative to https://github.com/HPC-AI-Team/MatRIS/blob/c16f569ca08e6905e91b64e2ee68614303e46f7f/matris/applications/base.py#L24. TorchSim is an engine for batched MLIP workflows that enables fast MD with replicas/repeats or batched geometry optimization with minimal conceptual overhead c.f. ASE.

@curtischong
Copy link
Collaborator

Just putting this here for documentation purposes - but this is similar to another PR that I wrote and closed earlier: #354

@thomasloux
Copy link
Collaborator

I added a torch-sim interface HPC-AI-Team/MatRIS#5
I didn't have time to test your PR, but it seems fine.

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

Labels

api API design discussions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants