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

PBC fix bundle #563

Draft
wants to merge 120 commits into
base: master
Choose a base branch
from
Draft

PBC fix bundle #563

wants to merge 120 commits into from

Conversation

evaleev
Copy link
Contributor

@evaleev evaleev commented Dec 4, 2024

  • nuclear density function fix
    • Added rscale
    • Replaced the R parameter with the cell : more readable and removes the assumption that the cell is a centered cube
    • Removes the hardcoded assumption that we're in PBC.
  • PBC handling in separated convolution and its applications cleaned up

JonathonMisiewicz and others added 24 commits November 7, 2024 11:46
…lution can be controlled by the user, if needed; the default is unchanged (truncate the expansion if periodic, not otherwise)
…sider periodic displacements since operator kernel includes lattice summation
@evaleev evaleev changed the title SeparatedConvolution: support for mixed (free+periodic) BC [jumbo] PBC fixes Dec 4, 2024
@evaleev evaleev changed the title [jumbo] PBC fixes PBC fix bundle Dec 4, 2024
@@ -1061,7 +1050,7 @@ namespace madness {
ops[mu].setfac(coeff(mu)/c);

for (std::size_t d=0; d<NDIM; ++d) {
ops[mu].setop(d,GaussianConvolution1DCache<Q>::get(k, expnt(mu)*width[d]*width[d], 0, isperiodicsum));
ops[mu].setop(d,GaussianConvolution1DCache<Q>::get(k, expnt(mu)*width[d]*width[d], 0, lattice_sum[d]));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robertjharrison while trying to deal with FunctionImpl::do_apply PBC issues I ended up tackling the mixed BC handling in SeparatedConvolution ... is there more to it than just toggling lattice sum flag for each setop? A pair of eyes on this commit would be good. @JonathonMisiewicz u2

@robertjharrison
Copy link
Contributor

robertjharrison commented Dec 4, 2024 via email

@evaleev evaleev force-pushed the jm/cleanup/nuclear-density-functor branch from 7373da9 to 5fa8afe Compare December 4, 2024 03:10
@evaleev
Copy link
Contributor Author

evaleev commented Dec 4, 2024

Prolly also how the operator displacements are generated in func defaults (?).

I went through that code, I think it's fine. I presume we only want to generate displacements for 2 cases only: all non-periodic and all-periodic, with mixed cases handled by the periodic displacements (optimizing for mixed case would just reduce the number of replacements). Or, do we want to generate the displacements for the specific boundary conditions FunctionDefaults was initialized for (i.e. assume the boundary conditions are immutable?).

I think the spherical shell-based screening logic should be OK in do_apply in either case. We won't be as efficient as could be but OK, let's chase correctness first.

Also if the aspect ratio is far from cubic (e.g., finite slab width is many multiples of the periodic width) then the sorting of displacements and the screening algorithm in do apply could be optimized (but I think this is just performance). I can read thru this later in the week.

I also touched that code, and should be OK.

… provides probing_displacement method to enable screening out the entire surface contribution
…acement as "used" if it survived estimated AND actial norms of its contribution
…o the entire surface of the boundary range, rather than to near the facet centers
…eriodic potential for the tight (nuclear-sized) densities
@evaleev evaleev force-pushed the jm/cleanup/nuclear-density-functor branch 2 times, most recently from 519ba6a to 9c310a0 Compare January 24, 2025 18:36
…is out of range filter function generalized to take Key pattern instead of Key; displacement is passed as an optional
@evaleev evaleev force-pushed the jm/cleanup/nuclear-density-functor branch from 9c310a0 to 63a3c85 Compare January 24, 2025 19:00
…ialFunctor with Poisson kernel range restricted, and optional support for periodic boundary conditions
displacements along nonfixed axes of finite radius did not take into account for the finite surface thickness.
nonperiodic standard displacements, although screened in units of spherical shells, are defined in cubic sets, this erroneously skipped displacements with one of the Cartesian components > bmax but distsq less than the max reached by standard displacements

implicitly lattice-summed range-restricted operator (RP)  still not correct, but at least its explicit version (RP2) is
…ndle "periodic" dimensions (those long which the operator has been lattice summed)

for periodic dimensions BoxSurfaceDisplacementRange only produces displacements that are unique modulo this period
@evaleev evaleev force-pushed the jm/cleanup/nuclear-density-functor branch from ee3ad43 to d78aee1 Compare February 5, 2025 14:00
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.

4 participants