-
Notifications
You must be signed in to change notification settings - Fork 15
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
Fixes regarding component ID #319
base: master
Are you sure you want to change the base?
Conversation
The checks fail since the PR is compared to the master branch. However, the master branch uses the new config files with the changed component IDs. This leads to a failure since the component ID specified in the How should we deal with this, @cniethammer @FG-TUM ? |
const size_t numComps = ensemble->getComponents()->size(); | ||
for(auto siteIter = query.begin(); siteIter; siteIter++) { | ||
Molecule molecule; | ||
xmlconfig.changecurrentnode(siteIter); | ||
int componentid; | ||
xmlconfig.getNodeValue("componentid", componentid); | ||
molecule.setComponent(ensemble->getComponent(componentid)); | ||
if (xmlconfig.getNodeValue("componentid", componentid)) { | ||
if ((componentid < 1) || (componentid > numComps)) { | ||
Log::global_log->error() << "[Basis] Specified componentid is invalid. Valid range: 1 <= componentid <= " << numComps << std::endl; | ||
Simulation::exit(1); | ||
} | ||
} else { | ||
Log::global_log->error() << "[Basis] No componentid specified. Set <componentid>!" << std::endl; | ||
Simulation::exit(1); | ||
} | ||
molecule.setComponent(ensemble->getComponent(componentid - 1)); // Internally stored in vector starting at index 0 |
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 kind of code should go in the ensemble->getComponent
method and not here.
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 am not sure if I fully understand your intention.
The actual bug fix is changing
ensemble->getComponent(componentid)
to ensemble->getComponent(componentid - 1)
since getComponent
uses the "internal" ID (starting with 0).
I added the checks to validate the user input via the xml file. This must be done here to print meaningful error messages.
A basic check in the getComponent
method is already kind of (only with Debug build) implemented:
ls1-mardyn/src/ensemble/EnsembleBase.h
Lines 81 to 84 in f785526
Component* getComponent(int cid) { | |
mardyn_assert(cid < static_cast<int>(_components.size())); | |
return &_components.at(cid); | |
} |
But this will print a more generic, not user-friendly error message.
Description
The components are internally stored as a vector. Therefore, the component IDs start with 0. However, in the specification of the components in the xml file, the ID starts with a 1. This leads to a bug in the Basis.cpp class, which doesn't convert the external ID (starting with 1) to the internally used one (starting with 0). This bug was also described in issue #315.
Besides the fix in Basis.cpp, also the specified componentIDs in the xml files were incremented to preserve the simulations' results.
Also added a check to validate the input of the IDs in the
<components>
section of the xml file.Resolved Issues