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

Changes for upcoming VAMPyR update #173

Merged
merged 15 commits into from
Sep 13, 2021
Merged

Changes for upcoming VAMPyR update #173

merged 15 commits into from
Sep 13, 2021

Conversation

stigrj
Copy link
Contributor

@stigrj stigrj commented Sep 5, 2021

  • There's now only one TreeIterator class with a parameter for parser type (Hilbert or Lebesgue) instead of the old derived classes HilbertIterator and LebesgueIterator. The main reason for this is for clarity when using it as the old class names were not very descriptive.
  • Removed the Gaussian's property of being "periodic", with a GaussExp member representing it's periodic images. Now there are just regular non-periodic Gaussians and a free function periodify(Gaussian<D>) (formerly known as make_gaussian_periodic) which returns a semi-periodic version of the Gaussian in the form of a GaussExp. So when working in a periodic setting you should first periodify your Gaussian and then continue with the resulting GaussExp. Not sure if this retains all the functionality of the old version, as I do not remember the ideas behind the old setup.
  • Some simple cleanups in the Gaussian classes.

and protect isVisibleAtScale and isZeroOnInterval.
Removed separate classes for Hilbert and Lebesgue iterators.
Now the type is given as argument to the generic TreeIterator.
Reason: easier to understand the TreeIterator name.
@stigrj stigrj requested a review from bjorgve September 5, 2021 12:12
@codecov
Copy link

codecov bot commented Sep 5, 2021

Codecov Report

Merging #173 (9fb67d5) into master (d1b186f) will increase coverage by 0.14%.
The diff coverage is 67.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #173      +/-   ##
==========================================
+ Coverage   65.59%   65.74%   +0.14%     
==========================================
  Files         173      171       -2     
  Lines       12401    12425      +24     
==========================================
+ Hits         8135     8169      +34     
+ Misses       4266     4256      -10     
Impacted Files Coverage Δ
src/functions/GaussFunc.h 66.66% <0.00%> (+16.66%) ⬆️
src/functions/GaussPoly.h 0.00% <ø> (ø)
src/operators/HelmholtzOperator.h 100.00% <ø> (ø)
src/operators/PoissonOperator.h 100.00% <ø> (ø)
src/treebuilders/add.cpp 85.71% <0.00%> (-14.29%) ⬇️
src/treebuilders/grid.cpp 73.61% <0.00%> (+4.04%) ⬆️
src/treebuilders/map.cpp 100.00% <ø> (ø)
src/trees/FunctionNode.h 66.66% <ø> (ø)
src/trees/FunctionTree.h 60.00% <0.00%> (-6.67%) ⬇️
src/trees/MWNode.cpp 79.82% <0.00%> (-0.22%) ⬇️
... and 45 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1b186f...9fb67d5. Read the comment docs.

@@ -47,8 +51,8 @@ namespace mrcpp {
* integral is conserved with respect to the integration limits.
*
*/
template <int D> std::shared_ptr<GaussExp<D>> function_utils::make_gaussian_periodic(const Gaussian<D> &inp, const std::array<double, D> &period, double nStdDev) {
auto gauss_exp = std::make_shared<GaussExp<D>>();
template <int D> GaussExp<D> function_utils::periodify(const Gaussian<D> &inp, const std::array<double, D> &period, double nStdDev) {
Copy link
Member

Choose a reason for hiding this comment

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

Add #include "functions/function_utils.h" in api/Gaussians. We need this in mrchem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, perhaps we should just re-introduce it as a member function of the Gaussian? I like this syntax better:

auto periodic_exp = gfunc.periodify(period);

Copy link
Member

Choose a reason for hiding this comment

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

I agree

@stigrj
Copy link
Contributor Author

stigrj commented Sep 9, 2021

@bjorgve can you think of any issue with the removal of the periodic Gaussians when it comes to its use in MRChem?

@bjorgve
Copy link
Member

bjorgve commented Sep 9, 2021

I'm looking into it now.

@bjorgve
Copy link
Member

bjorgve commented Sep 9, 2021

It didn't break any of my periodic tests. So I believe we are safe.

Copy link
Member

@bjorgve bjorgve left a comment

Choose a reason for hiding this comment

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

I'm happy with this

@stigrj
Copy link
Contributor Author

stigrj commented Sep 9, 2021

Great! I would still like to move the operator scale stuff from the MRA into the operators themselves, if possible. Looking into it now.

Also add kernel precision parameter for ConvolutionOperators and
fall back to old precision parameters for non-periodic operators.
@stigrj
Copy link
Contributor Author

stigrj commented Sep 10, 2021

Ok @bjorgve I think I managed to move the operator_scale operator_reach and operator_cutoff stuff out of MRA and into the MWOperator. Not fully confident that nothing breaks in periodic MRChem, especially with the apply_(near/far)_field stuff.

Old way of initializing periodic operator:

MultiResolutionAnalysis<3> MRA(pbc_world, basis);

// For negative scale operator
MRA.setOperatorScale(-5);
MRA.setOperatorReach(2);
MRA.setOperatorCutoff(1);

// For reaching operator:
MRA.setOperatorScale(0);
MRA.setOperatorReach(33);
MRA.setOperatorCutoff(32);

PoissonOperator(MRA, prec);

New way:

MultiResolutionAnalysis<3> MRA(pbc_world, basis);

// Negative scale operator
PoissonOperator(MRA, prec, -5, 1);

// Reaching operator
PoissonOperator(MRA, prec, 0, 32);

@bjorgve
Copy link
Member

bjorgve commented Sep 13, 2021

This is a nice change, this means we can choose a different reach/scale for the Poisson and Helmholtz operator. I have to find a clever way to set the reach in mrchem. But that's future Magnars problem. I'm happy with this pull request, go ahead and merge if it's ready.

There was a 1D special case of BoundingBox::getNode
implemented which was not periodified.
@stigrj
Copy link
Contributor Author

stigrj commented Sep 13, 2021

Ok, I think this is good now 🤞

@stigrj stigrj merged commit c900bb5 into MRChemSoft:master Sep 13, 2021
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.

None yet

2 participants