-
Notifications
You must be signed in to change notification settings - Fork 1
enh(metatomic): Vesin first pass #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
PicoCentauri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool. I took a quick look on one file. I will continue later.
src/gromacs/applied_forces/metatomic/metatomic_forceprovider.cpp
Outdated
Show resolved
Hide resolved
src/gromacs/applied_forces/metatomic/metatomic_forceprovider.cpp
Outdated
Show resolved
Hide resolved
src/gromacs/applied_forces/metatomic/metatomic_forceprovider.cpp
Outdated
Show resolved
Hide resolved
src/gromacs/applied_forces/metatomic/metatomic_forceprovider.cpp
Outdated
Show resolved
Hide resolved
src/gromacs/applied_forces/metatomic/metatomic_forceprovider.cpp
Outdated
Show resolved
Hide resolved
PicoCentauri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also need the TopologyPreprocessor to remove the LJ, charges etc as the NNpot and the QMMM module is doing it.
Otherwise it looks already almost functional!
as is done in nnpot
Co-authored-by: PicoCentauri <[email protected]>
Co-authored-by: luthaf <[email protected]>
|
First compiling build... (links too which is nice) |
Co-authored-by: PicoCentauri <[email protected]>
... and cleanup
| // "Broadcast" the static atom numbers to all ranks: all ranks must call this. | ||
| if (mpiComm_.isParallel()) | ||
| { | ||
| // resize/allocate on non-main ranks and broadcast raw bytes of vector | ||
| nblock_abc(mpiComm_.isMainRank(), mpiComm_.comm(), static_cast<std::size_t>(n_atoms), &atomNumbers_); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit unclear to me, it works under the assumption that main knows all the atom numbers, something I can't seem to figure out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like main will have the right set of atoms?
Co-authored-by: PicoCentauri <[email protected]>
|
@Luthaf this is ready for a drive-by, @PicoCentauri and I found a bug in the LJ forces for NNPot and this, which we're opening upstream. |
|
This is the issue for reference. https://gitlab.com/gromacs/gromacs/-/issues/5477#note_2876613421 |
| metatomic-input_group = System | ||
| metatomic-model = | ||
| metatomic-extensions = | ||
| metatomic-device = | ||
| metatomic-check_consistency = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming question, should these use metatomic_check_consistency or metatomic-check-consistency instead? It is a bit weird to have a mix of dash and underscore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all - should be gromacs style.
| // Its ONLY job is to update the `idxLookup_` map, which maps | ||
| // the contiguous ML atom index (0..n_atoms-1) to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you mean here. If all ranks have access to all positions, then we can give all atoms to the model in each rank, and use selected_atoms to pass the sparse index/what the model in a given rank should be doing.
| gromacs_scalar_type = torch::kFloat64; | ||
| } | ||
| // This blob_options is for reading GROMACS memory | ||
| auto blob_options = torch::TensorOptions().dtype(gromacs_scalar_type).device(torch::kCPU); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to check something to know if the data is on CPU? Or will the GPU data live in separate variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PicoCentauri and I shall investigate / ping lukas
| auto torch_cell = | ||
| torch::from_blob(&box_, { 3, 3 }, blob_options).to(this->dtype_).to(this->device_); | ||
|
|
||
| auto torch_pbc = torch::tensor(std::vector<uint8_t>{ 1, 1, 1 }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we sure that gromacs never have mixed pbc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I should do something like in the nnpot, i.e.
void TorchModel::preparePbcType(PbcType* pbcType)
{
torch::Tensor pbcTensor =
torch::tensor({ true, true, true }, torch::TensorOptions().dtype(torch::kBool));
if (*pbcType == PbcType::XY)
{
pbcTensor[2] = false;
}
else if (*pbcType != PbcType::Xyz)
{
GMX_THROW(InconsistentInputError(
"Option use_pbc was set to true, but PBC type is not supported."));
}
pbcTensor = pbcTensor.to(device_);
inputs_.push_back(pbcTensor);
}| auto torch_pbc = torch::tensor(std::vector<uint8_t>{ 1, 1, 1 }, | ||
| torch::TensorOptions().dtype(torch::kBool).device(this->device_)); | ||
| auto torch_types = | ||
| torch::tensor(atomNumbers_, torch::TensorOptions().dtype(torch::kInt32)).to(this->device_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is using the atomic number as atomic type, right? How would this handle CG simulations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think those need separate code, and have to register as supporting coarse-grained force fields
xref:
[1]: https://manual.gromacs.org/documentation/2019-rc1/reference-manual/functions/force-field.html#coarse-grained-force-fields
[2]: tpxio.cpp --> tpxv_RestrictedBendingAndCombinedAngleTorsionPotentials, /**< potentials for supporting coarse-grained force fields */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I think then also the atomNumbers_ will be different. I will check how we get them actually from the defined topology.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay they come from the atomtypes directive in the topolgy file. I checked a couple of predefined topologies that live here:
https://github.com/metatensor/gromacs/tree/main/share/top
and they seem to define the atomnumbers in the canonical way
gromacs/share/top/amber99sb-ildn.ff/ffnonbonded.itp
Lines 1 to 6 in eaa82eb
| [ atomtypes ] | |
| ; name at.num mass charge ptype sigma epsilon | |
| Br 35 79.90 0.0000 A 3.95559e-01 1.33888e+00 ; Converted from parm99.dat | |
| C 6 12.01 0.0000 A 3.39967e-01 3.59824e-01 | |
| CA 6 12.01 0.0000 A 3.39967e-01 3.59824e-01 | |
| CB 6 12.01 0.0000 A 3.39967e-01 3.59824e-01 |
The only thing we might want to filter out with a warning is atom number 0 as they use as these are dummy particles for some forcefields
gromacs/share/top/amber99sb-ildn.ff/ffnonbonded.itp
Lines 70 to 74 in eaa82eb
| ; MW=Dummy mass for tip4p/EW/5p water extra point charge | |
| MW 0 0.0000 0.0000 D 0.00000e+00 0.00000e+00 | |
| ; Dummy masses for rigid CH3 and NH3 groups | |
| MCH3 0 0.0000 0.0000 A 0.00000e+00 0.00000e+00 | |
| MNH3 0 0.0000 0.0000 A 0.00000e+00 0.00000e+00 |
0cc95c2 to
15ab6cc
Compare
Co-authored-by: Guillaume Fraux <[email protected]>
26a7592 to
4168411
Compare
Co-authored-by: Guillaume Fraux <[email protected]> Update src/gromacs/applied_forces/metatomic/metatomic_options.cpp Co-authored-by: Guillaume Fraux <[email protected]> Update src/gromacs/applied_forces/metatomic/metatomic_topologypreprocessor.h Co-authored-by: Guillaume Fraux <[email protected]> Update src/gromacs/applied_forces/metatomic/metatomic_topologypreprocessor.cpp Co-authored-by: Guillaume Fraux <[email protected]>
| metatomic-input_group = System | ||
| metatomic-model = | ||
| metatomic-extensions = | ||
| metatomic-device = | ||
| metatomic-check_consistency = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all - should be gromacs style.
Co-authored-by: Guillaume Fraux <[email protected]>
| #include "gromacs/utility/basedefinitions.h" | ||
| #include "gromacs/utility/exceptions.h" | ||
|
|
||
| #include "metatomic_forceprovider.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cause problems when building without metatomic because we will not find this header file.
cc @PicoCentauri
Reopened.
Very rough WIP. To be updated with https://gitlab.com/gromacs/gromacs/-/merge_requests/4323
(after tests run). Modeled (currently) after the nnpot and plumed/eon integrations via vesin.
For instructions on optimal usage / reproduction consider details here.