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

Piece wise beadstructure #474

Merged
merged 28 commits into from
Dec 13, 2019
Merged

Piece wise beadstructure #474

merged 28 commits into from
Dec 13, 2019

Conversation

JoshuaSBrown
Copy link
Contributor

Corrected include guard
Added pragma once
Added const correctness to variables
Added comments to method description
Simplified namespace TOOLS to tools
Switched from using pointer to references
Added const method accessor for getting bead

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.

I do not know, for me the whole ownership in csg is unclear.

include/votca/csg/beadstructure.h Outdated Show resolved Hide resolved
include/votca/csg/beadstructure.h Outdated Show resolved Hide resolved

/**
* \brief Return a vector of all the beads neighboring the index
**/
std::vector<T *> getNeighBeads(Index index);

TOOLS::Graph getGraph();
tools::Graph getGraph();
Copy link
Member

Choose a reason for hiding this comment

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

can it be const?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a copy there is no need to make const, and the copy is needs to be mutable so no, but good question.

Copy link
Member

@JensWehner JensWehner Dec 12, 2019

Choose a reason for hiding this comment

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

I meant Graph getGraph() const not const Graph getGraph() sorry for the confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, unfortunately, it cannot be at this point. The graph is only generated internally when it is needed. When the getGraph method is called it checks to see if the graph is up to date, if it is it does not generate anything, but if it isn't the graph is generated. I could auto update the graph every time a bead is added but this would be expensive, particularly if the graph is never actually needed.

Copy link
Member

Choose a reason for hiding this comment

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

ahh okay

include/votca/csg/beadstructure.h Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 11, 2019

Codecov Report

Merging #474 into master will increase coverage by 0.1%.
The diff coverage is 95%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #474     +/-   ##
========================================
+ Coverage      61%   61.2%   +0.1%     
========================================
  Files         128     129      +1     
  Lines        6299    6320     +21     
========================================
+ Hits         3847    3868     +21     
  Misses       2452    2452
Flag Coverage Δ
#gcc 61.2% <95%> (+0.1%) ⬆️
Impacted Files Coverage Δ
include/votca/csg/beadmotifalgorithms.h 100% <100%> (ø) ⬆️
src/libcsg/beadstructurealgorithms.cc 100% <100%> (ø) ⬆️
src/libcsg/beadmotif.cc 100% <100%> (ø) ⬆️
include/votca/csg/beadmotif.h 100% <100%> (ø) ⬆️
src/libcsg/beadmotifalgorithms.cc 92.4% <100%> (-0.1%) ⬇️
include/votca/csg/beadstructure.h 100% <100%> (+7.2%) ⬆️
src/libcsg/beadstructure.cc 93.5% <93.5%> (ø)

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 8982aba...e52a930. Read the comment docs.

@JoshuaSBrown
Copy link
Contributor Author

@JensWehner I hope this is more to your liking, I removed the internal pointers, so the beadstructure no longer runs the risk of accessing invalid pointers.

@JoshuaSBrown
Copy link
Contributor Author

@JensWehner hey does this satisfy your review? Can you approve?

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.

Yeah it is definitely an improvement

@junghans junghans merged commit 0ea596b into master Dec 13, 2019
@junghans junghans deleted the piece-wise-beadstructure branch December 13, 2019 19:24
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.

3 participants