search

CederGroupHub / chgnet

Pretrained universal neural network potential for charge-informed atomistic modeling https://chgnet.lbl.gov
https://doi.org/10.1038/s42256-023-00716-3
Other
220 stars 60 forks source link

Implement lattice transform to upper-triangular matrix for ASE Nose-Hoover thermostat #68

Closed tsihyoung closed 11 months ago

tsihyoung commented 11 months ago

0db053c Implement transformation of lattice to upper-triangular matrix for ASE Nose-Hoover thermostat dfd8bc4 Add verbose flag and explanation to upper_triangular_cell 1beaf1b fix ASE NPT does not accept externalstress=None f22adc1 Add tests for Nose-Hoover NVT and NPT MD a6480fd Change MD test numerical tolerance a976874 use numpy.testing.assert_allclose over np.isclose().all() for better error messages on failing tests cfba1c9 tweak upper_triangular_cell() doc str

tsihyoung commented 11 months ago

Thanks @janosh for reviewing the codes.

Do we need a test for NPT dynamics or for the transformation itself? If the latter is the case, the test might be not relevant enough to be included in this repository.

janosh commented 11 months ago

Could be I'm missing something. My understanding is you're addressing https://github.com/CederGroupHub/chgnet/pull/67#issuecomment-1709170273 from @BowenD-UCB. In which case this enables MD for systems that weren't supported before? If so, testing such a system would be good.

tsihyoung commented 11 months ago

Could be I'm missing something. My understanding is you're addressing #67 (comment) from @BowenD-UCB. In which case this enables MD for systems that weren't supported before? If so, testing such a system would be good.

Yes, that is what I meant. I will add testings for Nose-Hoover NVT and NPT dynamics.

BowenD-UCB commented 11 months ago

Could be I'm missing something. My understanding is you're addressing #67 (comment) from @BowenD-UCB. In which case this enables MD for systems that weren't supported before? If so, testing such a system would be good.

Yes, that is what I meant. I will add testings for Nose-Hoover NVT and NPT dynamics.

Thanks for helping addressing this issue! Yes, the test should include:

  1. showing a non-upper triangular structure being transformed into upper-triangular structure
  2. verify they're consistent by using pymatgen structure matcher
  3. and then running noose-hoover (both nvt and npt) on it
tsihyoung commented 11 months ago

Could be I'm missing something. My understanding is you're addressing #67 (comment) from @BowenD-UCB. In which case this enables MD for systems that weren't supported before? If so, testing such a system would be good.

Yes, that is what I meant. I will add testings for Nose-Hoover NVT and NPT dynamics.

Thanks for helping addressing this issue! Yes, the test should include:

  1. showing a non-upper triangular structure being transformed into upper-triangular structure
  2. verify they're consistent by using pymatgen structure matcher
  3. and then running noose-hoover (both nvt and npt) on it

I have added the required tests. By the way, although the default value of externalstress in ASE NPT class is None, it will cause ValueError as set_stress has not considered the case where externalstress is None. This has also been fixed.