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 to mamico plugin after mamico refactor #293

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

Conversation

amartyads
Copy link
Contributor

@amartyads amartyads commented Feb 26, 2024

Description

After the mamico coding week, some files and functions were renamed. The plugin is now fixed to use these new names.

Fixes Issue #290


bool BoundaryHandler::hasGlobalInvalidBoundary() const {
return std::any_of(_boundaries.begin(), _boundaries.end(), [](const auto &keyVal) {
const auto [dim, boundaryType] = keyVal;

Check notice

Code scanning / CodeQL

Unused local variable

Variable dim is not used.

bool BoundaryHandler::hasGlobalNonPeriodicBoundary() const {
return std::any_of(_boundaries.begin(), _boundaries.end(), [](const auto &keyVal) {
const auto [dim, boundaryType] = keyVal;

Check notice

Code scanning / CodeQL

Unused local variable

Variable dim is not used.
Comment on lines 89 to 137
switch (getGlobalWallType(currentDim)) {
case BoundaryUtils::BoundaryType::PERIODIC:
// nothing changes from normal ls1 behaviour, so leaving particles not touched by BoundaryHandler and
// are processed by DomainDecompBase::handleDomainLeavingParticles()
break;

case BoundaryUtils::BoundaryType::OUTFLOW:
[[fallthrough]];
case BoundaryUtils::BoundaryType::REFLECTING: {
// create region by using getInnerRegionSlab()
const auto [curWallRegionBegin, curWallRegionEnd] =
RegionUtils::getInnerRegionSlab(_localRegionStart, _localRegionEnd, currentDim, cutoff);
// grab an iterator from the converted coords
const auto particlesInRegion = moleculeContainer->regionIterator(
curWallRegionBegin.data(), curWallRegionEnd.data(), ParticleIterator::ONLY_INNER_AND_BOUNDARY);

// iterate through all molecules
for (auto moleculeIter = particlesInRegion; moleculeIter.isValid(); ++moleculeIter) {
// Calculate the change in velocity, which the leapfrog method will
// apply in the next velocity update to the dimension of interest.
const int currentDimInt = DimensionUtils::convertEnumToLS1DimIndex(currentDim);
const double halfTimestep = .5 * timestepLength;
const double halfTimestepByMass = halfTimestep / moleculeIter->mass();
const double force = moleculeIter->F(currentDimInt);
const double nextStepVelAdjustment = halfTimestepByMass * force;

// check if the molecule would leave the bounds
if (RegionUtils::isMoleculeLeaving(*moleculeIter, curWallRegionBegin, curWallRegionEnd, currentDim,
timestepLength, nextStepVelAdjustment)) {
if (getGlobalWallType(currentDim) == BoundaryUtils::BoundaryType::REFLECTING) {
const double currentVel = moleculeIter->v(currentDimInt);
// change the velocity in the dimension of interest such that when
// the leapfrog integrator adds nextStepVelAdjustment in the next
// velocity update, the final result ends up being the intended,
// reversed velocity: -(currentVel+nextStepVelAdjustment)
moleculeIter->setv(currentDimInt,
-currentVel - nextStepVelAdjustment - nextStepVelAdjustment);
} else { // outflow, delete the particle if it would leave
moleculeContainer->deleteMolecule(moleculeIter, false);
}
}
}
break;
}
default:
std::ostringstream error_message;
error_message << "BoundaryType::ERROR received in BoundaryHandler::processGlobalWallLeavingParticles!" << std::endl;
MARDYN_EXIT(error_message.str());
}

Check notice

Code scanning / CodeQL

Long switch case

Switch has at least one case that is too long: [REFLECTING (35 lines)](1).
cniethammer
cniethammer previously approved these changes Nov 26, 2024
Copy link
Contributor

@cniethammer cniethammer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not have time to test this - so just general remarks on the code.

Overall such huge PRs that include also out of topic modifications should be avoided and if not clearly state what was done in the PR description! Here I find new cmake and xml options beside general fixes and renamings of interfaces in the code ...

.gitignore Outdated Show resolved Hide resolved
cmake/modules/mamico.cmake Show resolved Hide resolved
examples/all-options.xml Outdated Show resolved Hide resolved
examples/all-options.xml Show resolved Hide resolved
src/utils/Math.h Outdated Show resolved Hide resolved
src/CMakeLists.txt Show resolved Hide resolved
src/Simulation.cpp Outdated Show resolved Hide resolved
src/particleContainer/ParticleContainer.h Outdated Show resolved Hide resolved
src/parallel/DomainDecompBase.cpp Show resolved Hide resolved
Co-authored-by: Christoph Niethammer <[email protected]>
cniethammer
cniethammer previously approved these changes Nov 26, 2024
Comment on lines +444 to +445
Log::global_log->error() << "Invalid boundary type! Please check the config file" << std::endl;
exit(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MARDYN_EXIT or something like this

Comment on lines +448 to +449
Log::global_log->info() << "Non-periodic boundaries not supported with overlappingP2P enabled! Exiting..." << std::endl;
exit(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

@@ -289,6 +290,23 @@ class DomainDecompBase: public MemoryProfilable {

virtual void printCommunicationPartners(std::string filename) const {};

/* Set the global boundary type for the _boundaryHandler object. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doc style

src/parallel/boundaries/BoundaryHandler.cpp Outdated Show resolved Hide resolved
src/parallel/boundaries/BoundaryHandler.cpp Outdated Show resolved Hide resolved
src/parallel/boundaries/RegionUtils.cpp Outdated Show resolved Hide resolved
src/parallel/boundaries/RegionUtils.cpp Outdated Show resolved Hide resolved
src/parallel/boundaries/RegionUtils.cpp Outdated Show resolved Hide resolved
src/utils/Math.h Outdated Show resolved Hide resolved
src/utils/Math.h Outdated Show resolved Hide resolved
remove inlines

Co-authored-by: FG-TUM <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix MaMiCo Plugin (MaMiCo refactoring February 2024)
5 participants