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 77 commits into
base: master
Choose a base branch
from
Draft

PBC fix bundle #563

wants to merge 77 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.

@evaleev evaleev force-pushed the jm/cleanup/nuclear-density-functor branch from 19346f6 to 475fad8 Compare January 1, 2025 19:25
…lattice_summed(), the latter assumes lattice summation applies to every term

SeparatedConvolution ctors check that every term has same lattice summation attributes
by replacing std::optional<unsigned int> it's more ergonomic and supports sort restrictor
@evaleev evaleev force-pushed the jm/cleanup/nuclear-density-functor branch from 991cdeb to 424d1e3 Compare January 7, 2025 19:25
evaleev added 14 commits January 8, 2025 17:33
…to support the case of zero displacements processed
…-thickness layer of displacements at the range boundary

only 1-d case supported/tested so far
…undary displacements

need support for n-d, dynamic setting for thickness, and restriction to relatively shallow levels. For n=5 each face in 3D is 2^{2(n-1)} * 3 layers = 768 displacements
…for NDIM>=3 limit the range surface contributions to n<=5 to avoid excessive costs
…tination point to skip unproductive displacements inside the range generator
@evaleev evaleev force-pushed the jm/cleanup/nuclear-density-functor branch from 952012e to 2236bd5 Compare January 8, 2025 22:35
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