Skip to content
This repository has been archived by the owner on Sep 27, 2021. It is now read-only.

Piece wize basebead #460

Merged
merged 27 commits into from
Dec 9, 2019
Merged

Piece wize basebead #460

merged 27 commits into from
Dec 9, 2019

Conversation

JoshuaSBrown
Copy link
Contributor

@JoshuaSBrown JoshuaSBrown commented Nov 23, 2019

Updating base bead from issue https://github.com/votca/csg/pull/381/files:

  • basebead identity is removed, and flattened to local variable
  • noexcept keyword is added to methods
  • const reference is used in methods
  • Molecule Item is removed and back pointer to molecule object is replaced with molecule id
  • pragma once added
  • header guards updated
  • default ids and types are set to tools::topology_constants

molecule_item_(nullptr),
mass_(0.0),
bead_position_set_(false){};
BaseBead() : topology_item_(nullptr), mass_(0.0), bead_position_set_(false){};
Copy link
Member

Choose a reason for hiding this comment

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

you can initialize them below in th elist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

I would set the values of these things not in the constructor but when they are declared in the private section of the class.

Copy link
Member

Choose a reason for hiding this comment

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

change please

@junghans
Copy link
Member

junghans commented Dec 6, 2019

[ 48%] Building CXX object xtp/src/libxtp/CMakeFiles/votca_xtp.dir/gwbse/bse_operator.cc.o
cd /home/votca/votca/build/xtp/src/libxtp && /usr/lib64/ccache/g++  -DDEBUG -Dvotca_xtp_EXPORTS -I/home/votca/votca/build/xtp/src/libxtp -I/home/votca/votca/xtp/include -I/home/votca/votca/build/xtp/include -I/home/votca/votca/csg/include -I/home/votca/votca/tools/include -I/home/votca/votca/build/tools/include -isystem /usr/include/eigen3  -Wall -Wextra -Wpedantic -Wshadow -Wconversion -Werror -fopenmp -g -fPIC   -std=c++14 -o CMakeFiles/votca_xtp.dir/gwbse/bse_operator.cc.o -c /home/votca/votca/xtp/src/libxtp/gwbse/bse_operator.cc
�[91m/home/votca/votca/csgapps/orientcorr/orientcorr.cc: In member function 'bool MyWorker::FoundPair(votca::csg::Bead*, votca::csg::Bead*, const Vector3d&, double)':
/home/votca/votca/csgapps/orientcorr/orientcorr.cc:227:11: error: 'class votca::csg::Bead' has no member named 'getMolecule'; did you mean 'getMoleculeId'?
  227 |   if (b1->getMolecule() == b2->getMolecule()) {
      |           ^~~~~~~~~~~
      |           getMoleculeId
/home/votca/votca/csgapps/orientcorr/orientcorr.cc:227:32: error: 'class votca::csg::Bead' has no member named 'getMolecule'; did you mean 'getMoleculeId'?
  227 |   if (b1->getMolecule() == b2->getMolecule()) {
      |                                ^~~~~~~~~~~
      |                                getMoleculeId
�[0m�[91mgmake[2]: *** [csgapps/CMakeFiles/orientcorr.dir/build.make:66: csgapps/CMakeFiles/orientcorr.dir/orientcorr/orientcorr.cc.o] Error 1
�[0mgmake[2]: Leaving directory '/home/votca/votca/build'
�[91mgmake[1]: *** [CMakeFiles/Makefile2:2966: csgapps/CMakeFiles/orientcorr.dir/all] Error 2
gmake[1]: *** Waiting for unfinished jobs....

@JoshuaSBrown
Copy link
Contributor Author

Requires that votca/votca#235 is merged.

@junghans
Copy link
Member

junghans commented Dec 9, 2019

This one needs to be merged next, @JensWehner please review!

@codecov
Copy link

codecov bot commented Dec 9, 2019

Codecov Report

Merging #460 into master will increase coverage by <.1%.
The diff coverage is 93.3%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #460     +/-   ##
=======================================
+ Coverage      61%    61%   +<.1%     
=======================================
  Files         129    128      -1     
  Lines        6298   6299      +1     
=======================================
+ Hits         3845   3846      +1     
  Misses       2453   2453
Flag Coverage Δ
#gcc 61% <93.3%> (ø) ⬆️
Impacted Files Coverage Δ
src/libcsg/map.cc 43.9% <0%> (+0.3%) ⬆️
src/libcsg/molecule.cc 90.9% <100%> (ø) ⬆️
src/libcsg/modules/io/lammpsdatareader.cc 84.9% <100%> (+0.1%) ⬆️
src/libcsg/exclusionlist.cc 58% <100%> (ø) ⬆️
include/votca/csg/basebead.h 100% <100%> (+4.7%) ⬆️
include/votca/csg/molecule.h 85.7% <0%> (-1%) ⬇️
include/votca/csg/bead.h 70.5% <0%> (-0.9%) ⬇️
src/tools/csg_map.cc 54.8% <0%> (-0.7%) ⬇️
src/libcsg/topology.cc 47.1% <0%> (-0.3%) ⬇️
include/votca/csg/pdbwriter.h 0% <0%> (ø) ⬆️
... and 2 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 8fa5f72...e237f6e. Read the comment docs.

molecule_item_(nullptr),
mass_(0.0),
bead_position_set_(false){};
BaseBead() : topology_item_(nullptr), mass_(0.0), bead_position_set_(false){};
Copy link
Member

Choose a reason for hiding this comment

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

change please

src/libcsg/exclusionlist.cc Show resolved Hide resolved
Copy link
Member

@JensWehner JensWehner left a comment

Choose a reason for hiding this comment

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

can you please move the defualt values from the constructor to the private declaration

@JoshuaSBrown
Copy link
Contributor Author

@JensWehner they have been, which ones are you referring too?

Copy link
Member

@JensWehner JensWehner left a comment

Choose a reason for hiding this comment

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

@JoshuaSBrown I am sorry, I was confused. Good work.

@JensWehner JensWehner merged commit a7181e9 into master Dec 9, 2019
@JensWehner JensWehner deleted the piece-wize-basebead branch December 9, 2019 21:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants