Skip to content
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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fixes regarding component ID #319

wants to merge 6 commits into from

Conversation

HomesGH
Copy link
Contributor

@HomesGH HomesGH commented Jun 4, 2024

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

@HomesGH HomesGH requested a review from cniethammer June 4, 2024 10:47
@HomesGH
Copy link
Contributor Author

HomesGH commented Jun 4, 2024

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 <basis> can now be equal to the number of components (which is invalid in the master branch).

How should we deal with this, @cniethammer @FG-TUM ?

@HomesGH HomesGH added bug Something isn't working CI labels Jun 4, 2024
Comment on lines +23 to +37
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
Copy link
Contributor

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.

Copy link
Contributor Author

@HomesGH HomesGH Jun 5, 2024

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:

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in Basis.cpp regarding component ID
2 participants