-
Notifications
You must be signed in to change notification settings - Fork 63
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
base: master
Are you sure you want to change the base?
PBC fix bundle #563
Conversation
… nuclei programmatically
…lution can be controlled by the user, if needed; the default is unchanged (truncate the expansion if periodic, not otherwise)
…istances using distsq_periodic
…sider periodic displacements since operator kernel includes lattice summation
src/madness/mra/operator.h
Outdated
@@ -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])); |
There was a problem hiding this comment.
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
Huh.
Prolly also how the operator displacements are generated in func defaults
(?). 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.
…On Tue, Dec 3, 2024, 20:10 Eduard Valeyev ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/madness/mra/operator.h
<#563 (comment)>
:
> @@ -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]));
@robertjharrison <https://github.com/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 <https://github.com/JonathonMisiewicz>
u2
—
Reply to this email directly, view it on GitHub
<#563 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABZSAPN7RWJ5AF2FQT2FDUL2DZI7ZAVCNFSM6AAAAABS67KN6WVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINZXGA2TKMBVG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
7373da9
to
5fa8afe
Compare
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
I also touched that code, and should be OK. |
…al, not plain int SeparatedConvolution also provides direct access to kernel range
19346f6
to
475fad8
Compare
…lattice_summed(), the latter assumes lattice summation applies to every term SeparatedConvolution ctors check that every term has same lattice summation attributes
…splacements possible
by replacing std::optional<unsigned int> it's more ergonomic and supports sort restrictor
991cdeb
to
424d1e3
Compare
…rather than a copy
…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
952012e
to
2236bd5
Compare
rscale