Skip to content

Implement local force capping#76

Open
J-Hizzle wants to merge 4 commits intoXzzX:mainfrom
J-Hizzle:implement-local-force-capping
Open

Implement local force capping#76
J-Hizzle wants to merge 4 commits intoXzzX:mainfrom
J-Hizzle:implement-local-force-capping

Conversation

@J-Hizzle
Copy link
Contributor

@J-Hizzle J-Hizzle commented Feb 3, 2026

Solves #72.

I introduced a BinaryPred in analogy to the UnaryPred that was introduced in ThermodynamicForce.hpp:89.

The BinaryPred is not immediately necessary. It would also be possible to realize a local capping with a UnaryPred that would be checked once for the outer loop of LennardJones.apply_if over the atoms and once for the inner loop over the neighbor list. However, this way, the partition into AT + near Delta (= no capping), far Delta (= capping) and TR (= no interaction) is straight forward as well, since the user can have AND and OR conditions on the positions of the two particles.

At least during testing on my desktop computer, the changes did not impair the observed performance.

@J-Hizzle
Copy link
Contributor Author

J-Hizzle commented Feb 3, 2026

On a side note: When running a simulation with multiple potentials applied (such as LennardJones and DSF), the current implementation loops over the views contained in the atoms class for each potential individually. This means that there is as many loops over the atoms as there are different potentials applied. Would it be more performant to have one loop and call the potential functions requested from the user within this loop?

So, rather than:
apply LJ {
for atoms {
for neighbors {
apply LJ forces
}
}
}
apply DSF {
for atoms {
for neighbors {
apply DSF forces
}
}
};

One could do:
apply potentials {
for atoms {
for neighbors {
apply LJ forces
apply DSF forces
}
}
}.

@XzzX
Copy link
Owner

XzzX commented Feb 3, 2026

On a side note: When running a simulation with multiple potentials applied (such as LennardJones and DSF), the current implementation loops over the views contained in the atoms class for each potential individually. This means that there is as many loops over the atoms as there are different potentials applied. Would it be more performant to have one loop and call the potential functions requested from the user within this loop?

So, rather than: apply LJ { for atoms { for neighbors { apply LJ forces } } } apply DSF { for atoms { for neighbors { apply DSF forces } } };

One could do: apply potentials { for atoms { for neighbors { apply LJ forces apply DSF forces } } }.

yes.

@XzzX
Copy link
Owner

XzzX commented Feb 23, 2026

Can we separate the LJ change from the example, aka two PRs?

@XzzX
Copy link
Owner

XzzX commented Feb 23, 2026

I do not know if it is clear what the predicate takes as Arguments. What about

using TwoPositionsPredicate = std::predicate<
                         const real_t, //< pos 1 x
                         const real_t, //< pos 1 y
                         const real_t, //< pos 1 z
                         const real_t, //< pos 2 x
                         const real_t, //< pos 2 y
                         const real_t  //< pos 2 z
>;

?
Not sure if I like the name.

data::Subdomain({0_r, 0_r, 0_r}, {config.Lx, config.Lx, config.Lx}, config.neighborCutoff);

// calculate volume of the simulation domain
const auto volume = subdomain.diameter[0] * subdomain.diameter[1] * subdomain.diameter[2];
Copy link
Owner

Choose a reason for hiding this comment

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

We can make this a member of the Subdomain, aka subdomain.getVolume();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. On it.

@XzzX
Copy link
Owner

XzzX commented Feb 23, 2026

I like the simulation script. I think it is very readable and understandable.

@XzzX
Copy link
Owner

XzzX commented Feb 23, 2026

However, it is not doing multi res, so perhaps we should remove it from the name.

@J-Hizzle
Copy link
Contributor Author

However, it is not doing multi res, so perhaps we should remove it from the name.

Good point. What about "02_tracerArgon_localCap"?

J-Hizzle added a commit to J-Hizzle/mrmd that referenced this pull request Feb 23, 2026
@J-Hizzle
Copy link
Contributor Author

Can we separate the LJ change from the example, aka two PRs?

Yes, I did that in #80.

@J-Hizzle
Copy link
Contributor Author

I do not know if it is clear what the predicate takes as Arguments. What about

using TwoPositionsPredicate = std::predicate<
                         const real_t, //< pos 1 x
                         const real_t, //< pos 1 y
                         const real_t, //< pos 1 z
                         const real_t, //< pos 2 x
                         const real_t, //< pos 2 y
                         const real_t  //< pos 2 z
>;

? Not sure if I like the name.

Hmm, I see the point. We can change it.

@J-Hizzle
Copy link
Contributor Author

I do not know if it is clear what the predicate takes as Arguments. What about

using TwoPositionsPredicate = std::predicate<
                         const real_t, //< pos 1 x
                         const real_t, //< pos 1 y
                         const real_t, //< pos 1 z
                         const real_t, //< pos 2 x
                         const real_t, //< pos 2 y
                         const real_t  //< pos 2 z
>;

? Not sure if I like the name.

I think I solved this in 2fbbabb on PR #80. I had to generalize the predicates to concepts and I did it in datatypes.hpp.

XzzX added a commit that referenced this pull request Feb 25, 2026
J-Hizzle added a commit to J-Hizzle/mrmd that referenced this pull request Feb 25, 2026
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.

2 participants