Conversation
GardevoirX
force-pushed
the
heat-flux-wrapper
branch
from
bc49697 to
04efdd2
Compare
GardevoirX
force-pushed
the
heat-flux-wrapper
branch
2 times, most recently
from
35c184e to
d684615
Compare
GardevoirX
force-pushed
the
heat-flux-wrapper
branch
from
4d75155 to
677cd19
Compare
|
I think this is now ready for reviewing. Though the current results with In terms of the implementation, thie current state hasn't reached an ideal state. The calculation of the The second thing is how we do "unfolding" in
Tests showed that the second way can generate fewer atoms and thus can save runtime from the model inference, while an extra NL calculation also costs time. My intuition is that which one is faster depends on the system size. Also maybe we need a tutorial about how to use this wrapper. |
| components=[Labels(["xyz"], torch.arange(3, device=device).reshape(-1, 1))], | ||
| properties=Labels(["heat_flux"], torch.tensor([[0]], device=device)), | ||
| ) | ||
| results["extra::heat_flux"] = TensorMap( |
There was a problem hiding this comment.
Instead of having a custom output here, it would be better to also add heat_flux as a standard output =)
| HeatFluxWrapper, | ||
| _check_collisions, | ||
| _collisions_to_replicas, | ||
| _generate_replica_atoms, | ||
| _unfold_system, | ||
| _wrap_positions, |
There was a problem hiding this comment.
I don't love importing all private functions. The tests should test the public API, not the private implementation
There was a problem hiding this comment.
hmm then we would only have smoke tests?
|
|
||
| self._requested_inputs = { | ||
| "masses": ModelOutput(quantity="mass", unit="u", per_atom=True), | ||
| "velocities": ModelOutput(quantity="velocity", unit="A/fs", per_atom=True), |
There was a problem hiding this comment.
This might not match the positions units required by the underlying model
There was a problem hiding this comment.
Also, I'm not sure what happens if the underlying model already request these inputs but with a different unit. We should at least check & error.
| per_atom=False, | ||
| ) | ||
| outputs = self._model.capabilities().outputs.copy() | ||
| outputs["extra::heat_flux"] = hf_output |
There was a problem hiding this comment.
If we have multiple energy variant in the underlying model, I think this should expose multiple heat_flux variants with the same names/descriptions.
| } | ||
|
|
||
| hf_output = ModelOutput( | ||
| quantity="heat_flux", |
There was a problem hiding this comment.
This does not exist right now, but we should add it!
2 participants
Closes #104
This is still in a very early stage and subjected to change.
TODO:
Remove Replace the dependency onvesin.metatomicwithvesin.torchand rebuild neighbor list from the un-unfolded system (Hard to build the neighboring information between the replica atoms, e.g. atoms in different image boxes. Andvesin.metatomicis not supposed to be used inmetatomic.)Contributor (creator of pull-request) checklist
Reviewer checklist