-
Notifications
You must be signed in to change notification settings - Fork 282
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
New warnings for METAD adn WALKERS_MPI #931
Conversation
Thanks @Iximiel ! A few comments:
|
//If this Action is not compiled with MPI the user is informed and we exit gracefully | ||
if(walkers_mpi_){ | ||
plumed_assert(Communicator::PlumedHasMPI) << "Invalid walkers configuration: WALKERS_MPI flag requires MPI compilation"; | ||
plumed_assert(Communicator::initialized()) << "Invalid walkers configuration: WALKERS_MPI needs the communicator correctly initialized."; |
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.
Here I think you could just remove the first test.
Beside this, the way to raise the warnings is correct!!
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.
In my first draft i had a single error message with "MPI is not initialized or plumed is not compiled with MPI", but I think is more clear for the user to have two more specialized messages, and easier to test for the developers
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 you are right
Yes, but I set up the test as a completely external code, so when I compile the test I'm not seeing the compile-time definitions of PLUMED, or at least, I do not know how to access to them.
My bad, now
when
the test is in not in src/, but in unittest. Since is very far from the standard you use in the regtest directory, I though it was a good idea to keep it separated before the full merge |
Hi @Iximiel, this is good with me! as a side note, the test that is failing now is a "style check" one. We enforce some basic styling in the code and you can get it fixed by running "make astyle" in src/ and commit again |
@Iximiel regarding this point:
You are right. However, since we use some of the compile-time definitions of PLUMED in header files (actually very few, but Given this, once you bring your test within the regtest library, this is designed to make the macros available when compiling regtests. So, I think it's fine using these macros in regtests. But it's also ok (and I think better) to write a small function that makes the bool available for outside usage. |
another stylistic convention that we have is: classes upper case, functions lower case. So perhaps it's more consistent to have :-) |
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #931 +/- ##
==========================================
+ Coverage 85.73% 85.82% +0.09%
==========================================
Files 600 600
Lines 53472 54722 +1250
==========================================
+ Hits 45843 46964 +1121
- Misses 7629 7758 +129
☔ View full report in Codecov by Sentry. |
I have moved the new test in the regtest folder and I did some addition to make it work via I runned astyle on the src, I think this can exit the draft state |
* Test with and without MPI * WALKERS_MPI now throws without MPI initializated * test and error message now are more clear * clearer comments in the tests * added the last newline to the new files * PlumedHasMPI is now a function * camelCased plumedHasMPI * style aligned * moved files * created compiledtest type * compiled tests now support align with the other tests * Moved the new test in the basic directory * now the new test is completely aligned with the old ones * removed compiled test * updated the reference --------- Co-authored-by: Daniele Rapetti <daniele.rapetti@sissa.it>
Description
This should resolve #926:
METAD now throws if the user ask for WALKERS_MPI without a MPI installation and also if the MPI routines are not set up.
I wanted the changes testable so:
This is a draft because I wanted to show the solution that I have found, and I have not added the new test to the github workflow
Type of contribution
Copyright
COPYRIGHT
file with the correct license information. Code should be released under an open source license. I also used the commandcd src && ./header.sh mymodulename
in order to make sure the headers of the module are correct.Tests