Sorted ActionSetup and added minimal atoms class to ensure that eggs with load keep working

gtribello commented 4 months ago

Description

These are some changes in response to our discussions on 6th March.

I have moved the actions for creating constants to generic and moved DRMSD to colvar. Constant also no longer inherits from ActionSetup. ActionSetup thus has the same role as it did in the earlier versions of the code (it is just for LOAD and UNITS). Note that ActionForInterface actions are created before the plumed input is read in so some changes to ActionSetup are still necessary.
I have created a small class that contains the methods from Atoms that people use in the eggs use the PLUMED action LOAD as we discussed. I have also added a regtest for this functionality.

Target release

I would like my code to appear in release 2.10

Type of contribution

[x] changes to code or doc authored by PLUMED developers, or additions of code in the core or within the default modules
[ ] changes to a module not authored by you
[ ] new module contribution or edit of a module authored by you

Copyright

[x] I agree to transfer the copyright of the code I have written to the PLUMED developers or to the author of the code I am modifying.

[ ] the module I added or modified contains a COPYRIGHT file with the correct license information. Code should be released under an open source license. I also used the command cd src && ./header.sh mymodulename in order to make sure the headers of the module are correct.

Tests

[x] I added a new regtest or modified an existing regtest to validate my changes.
[x] I verified that all regtests are passed successfully on GitHub Actions.

codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 75.00000% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 83.34%. Comparing base (6a56f54) to head (4776336). Report is 4 commits behind head on master.

:exclamation: Current head 4776336 differs from pull request most recent head 842a5b9. Consider uploading reports for the commit 842a5b9 to get more accurate results

Files	Patch %	Lines
src/tools/DLLoader.cpp	0.00%	3 Missing :warning:
src/generic/Constant.cpp	50.00%	2 Missing :warning:
src/cltools/Benchmark.cpp	0.00%	1 Missing :warning:
src/core/ActionSetup.cpp	75.00%	1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #1036 +/- ## ========================================== - Coverage 83.34% 83.34% -0.01% ========================================== Files 618 618 Lines 59100 59119 +19 ========================================== + Hits 49258 49272 +14 - Misses 9842 9847 +5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

GiovanniBussi commented 4 months ago

Note that ActionForInterface actions are created before the plumed input is read in so some changes to ActionSetup are still necessary.

For this issue I was adding the ActionAnyorder base class, it is used by classes that can go before UNITS. But I now see that it is also a base for MOLINFO, which is not good because it looks like it uses units :-(

    if( ! pdb.read(reference,plumed.getAtoms().usingNaturalUnits(),0.1/plumed.getAtoms().getUnits().getLength()))plumed_merror("missing input file " + reference );

Maybe this is wrong?

GiovanniBussi commented 4 months ago

In addition, I think you should replace depreciated with deprecated everywhere

gtribello commented 4 months ago

I have fixed the spelling of deprecated (embarrassed face). My defence is going to be that it is a word that I have only heard Italians using and they have been pronouncing it wrong!

On the ActionForInterface thing, I think it makes sense that ActionSetup allows ActionForInterface actions before them as these are the PUT and DomainDecomposition actions that get data from the MD code. These actions have to be created before you start reading teh plumed input file, which begins with the setup actions.

GiovanniBussi commented 3 months ago

On the ActionForInterface thing, I think it makes sense that ActionSetup allows ActionForInterface actions before them as these are the PUT and DomainDecomposition actions that get data from the MD code. These actions have to be created before you start reading teh plumed input file, which begins with the setup actions.

Good point so maybe we should enforce that order?

gtribello commented 3 months ago

I wouldn't enforce anything more than we have. A person using PLUMED from python might want to pass some data to plumed, get it back from plumed and then pass some more data to PLUMED after some treatment in python. I thus would allow people to create ActionWithInterface objects after they have created the ActionSetup objects as well.

Notice furthermore that a GET action is an ActionWithInterface object and that definitely comes after the SETUP.

GiovanniBussi commented 3 months ago

OK makes sense.

I am still worried by UNITS and how it interact with other actions.

Maybe we should enforce some check? For instance, whenever we call the Units method we mark something as "called" and if then one tries to replace Units the code crashes. Because the way I was originally implementing this check (with ActionSetup) is not very robust

GiovanniBussi commented 3 months ago

@gtribello meanwhile I merge this

plumed / plumed2