MRChemSoft / mrcpp

MultiResolution Computation Program Package
GNU Lesser General Public License v3.0
12 stars 19 forks source link

Cleanups in convolution operators #196

Closed stigrj closed 2 years ago

stigrj commented 2 years ago

This will give VAMPyR the ability to build ConvolutionOperator from arbitrary GaussExp<1>

codecov[bot] commented 2 years ago

Codecov Report

Merging #196 (c0c8582) into master (0e079a9) will decrease coverage by 0.68%. The diff coverage is 83.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #196      +/-   ##
==========================================
- Coverage   63.86%   63.17%   -0.69%     
==========================================
  Files         172      174       +2     
  Lines       13056    13077      +21     
==========================================
- Hits         8338     8262      -76     
- Misses       4718     4815      +97     
Impacted Files Coverage Δ
src/operators/ABGVOperator.h 100.00% <ø> (ø)
src/operators/BSOperator.h 100.00% <ø> (ø)
src/operators/DerivativeConvolution.cpp 0.00% <0.00%> (ø)
src/operators/DerivativeKernel.h 0.00% <0.00%> (ø)
src/operators/HelmholtzOperator.h 100.00% <ø> (ø)
src/operators/IdentityConvolution.h 100.00% <ø> (ø)
src/operators/PHOperator.h 100.00% <ø> (ø)
src/operators/PoissonOperator.cpp 43.47% <41.17%> (-56.53%) :arrow_down:
src/operators/HelmholtzOperator.cpp 56.52% <57.89%> (-43.48%) :arrow_down:
src/operators/MWOperator.h 80.00% <77.77%> (-20.00%) :arrow_down:
... and 29 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 0e079a9...c0c8582. Read the comment docs.

robertodr commented 2 years ago

For OperatorTreeVector, does the 2nd CTOR of unique_ptr suffice? I recall wrapping smart pointers into STL containers as having some pitfalls...

stigrj commented 2 years ago

Which is the 2nd CTOR of unique_ptr?

robertodr commented 2 years ago

https://en.cppreference.com/w/cpp/memory/unique_ptr

template <
    class T,
    class Deleter
> class unique_ptr<T[], Deleter>;
stigrj commented 2 years ago

Sorry, I do not understand the problem

stigrj commented 2 years ago

You mean it's unsafe to do the following?

std::vector<std::unique_ptr<OperatorTree>> oper_vec;
auto o_tree = std::make_unique<OperatorTree>(mra, prec);
oper_vec.push_back(std::move(o_tree));
robertodr commented 2 years ago

You mean it's unsafe to do the following?

std::vector<std::unique_ptr<OperatorTree>> oper_vec;
auto o_tree = std::make_unique<OperatorTree>(mra, prec);
oper_vec.push_back(std::move(o_tree));

yes, or rather, there used to be a warning against using smart pointers like that. But some Googling shows it's pre-C++11 thing.

stigrj commented 2 years ago

I will update the doc string before merging