Fix `adjust_momenta` of `FixCom`

First MR relevant to #1285 (closed).

FixCom should make the center-of-mass velocity zero (This is also consistent with the RATTLE algorithm), but at the moment, this is not correctly implemented. This MR fixes this issue.

The first commit is for adding the test to demonstrate the issue.

The second commit fixes the issue above.

The third commit is to fix the default adjust_momenta in the (essentially abstract) class FixConstraint. At the moment, adjust_momenta is simply redirected to adjust_forces. This is however reasonable only for some type of subclasses (IndexedConstraint etc.) and in general not (FixBondLengths etc). To let new constraint developers notice about adjust_momenta I would make it as an abstract method (to force the developers to always implement this method). This commit may be optional but I feel it reasonable.

Note: Just to preserve the present behavior, I also re-implement adjust_momenta for some subclasses in the same way as the original FixConstraint (i.e. redirected to adjust_forces). I am however not immediately sure if some of them are reasonable (or correct) (e.g. for ExternalForce, should adjust_momenta really be the same as adjust_forces?) Since the motivation of this MR is just to fix FixCom, I will postpone thinking of this point...

changed title from Fix adjust_momenta if FixCom to Fix adjust_momenta of FixCom

added 108 commits

• 0efdd7c1...c6274c82 - 105 commits from branch ase:master
• d0915967 - Add test for `adjust_momenta` of `FixCom`
• 7ac138cb - Add `adjust_momenta` to `FixCom`
• 5fb43fd5 - Make `FixConstraint` a subclass of `ABC`

Compare with previous version

added 1 commit

• 72e5db40 - Make `FixConstraint` a subclass of `ABC`

Compare with previous version

added 2 commits

• 1d57d9ad - Move reference to `adjust_forces`
• 603bc455 - Minor-refactor `FixCom`

Compare with previous version

mentioned in issue #1300 (closed)

Hi @schiotz, would you be able to have a look please? It sounds very reasonable that the momenta should be updated by constraints, but sometimes thermostats and other MD things have fiendish subtleties.

Of course all the tests pass so in principle all should be good.

Maybe, @yuzie007, if there are implementations that we are not sure about, then it would be good to add comments inside the code pointing to this discussion.

added 4 commits

• 1c299178 - Make `FixConstraint` a subclass of `ABC`
• 8261a895 - Move reference to `adjust_forces`
• 7c266748 - Minor-refactor `FixCom`
• 34201923 - Update releasenotes.rst

Compare with previous version

Thank you @askhl for your kind review as always, and I would be very happy to have review also from @schiotz.

Following the suggestion, I made following changes for to let FixConstraint inherit ABC.

• ExternalForce, MirrorForce, MirrorTorque probably just modifies forces or torques and not for MD (in the sense of the RATTLE algorithm). So probably adjust_momenta should do noting in there (like Hookean). So I modified adjust_momenta for them to do nothing.

• For FixedMode, FixInternals, FixSymmetry, maybe adjust_momenta should not be adjusted in the same way as adjust_forces, I so added TODO (Note just for reminder: I do not modify any code behavior here since previously, and no backward compatibility is broken due to the MR. I just take the default behavior from previous FixConstraint, which could just be wrong since previously.)

In case you worry about the changes followed by the ABC inheritance, we can revert the changes not using abstractmethod.

Hi @schiotz, how is everything? Would you be able to have a look?

added 158 commits

• 34201923...9fb8dd74 - 153 commits from branch ase:master
• 9616df13 - Add test for `adjust_momenta` of `FixCom`
• a8b256f3 - Add `adjust_momenta` to `FixCom`
• bc374444 - Move reference to `adjust_forces`
• f24111b7 - Minor-refactor `FixCom`
• a6eb8ed9 - Update releasenotes.rst

Compare with previous version

Hopefully for easier review, I have omitted the abstraction of FixConstraint from the MR. (Maybe this will be a separate MR.) I believe the present MR is a simple and reasonable fix and wish to have it until the next public release of ASE:)

Thank you very much @yuzie007, merging.

You're welcome to suggest the other API change in a separate issue or MR. I think it is a good idea to be more explicit about momenta.

merged

mentioned in commit e0569c49