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

PBC fix bundle #563

wants to merge 53 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 bc584e9 to 488a603 Compare December 12, 2024 17:23
issmall refers to rnlij, not rnlp! But the range-restriction logic in outside_of_range referred to rnlp. To avoid confusion, renamed outside_the_range to rnlp_is_zero. Documented
@evaleev evaleev force-pushed the jm/cleanup/nuclear-density-functor branch from 2eec9b0 to 9e92446 Compare December 13, 2024 14:07
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