AddContainer updates ID

[rubengerritsen]

fixes #597

Or at least so I think, there are two assumptions:

• That it is fine to make a copy of the incoming atoms
• That the ID's of incoming molecules always start from zero

@JensWehner are these valid assumptions?

[JensWehner]

Suggested change

	for (auto at : container._atomlist) {
	for (const auto& at : container._atomlist) {

[JensWehner]

@rubengerritsen your assumptions are correct as far as I know.

[JensWehner]

actually I am not sure about other T like atom or staticsite. I am not sure if this does not break other template arguements.

[codecov]

Codecov Report

Merging #599 (032d9e4) into master (312a8a9) will increase coverage by 0.0%.
The diff coverage is 50.0%.

@@          Coverage Diff           @@
##           master    #599   +/-   ##
======================================
  Coverage    51.5%   51.5%           
======================================
  Files         302     302           
  Lines       28815   28819    +4     
======================================
+ Hits        14846   14849    +3     
- Misses      13969   13970    +1     

Impacted Files	Coverage Δ
include/votca/xtp/atomcontainer.h	97.8% <ø> (-0.1%)	⬇️
src/libxtp/gwbse/gwbse.cc	0.0% <0.0%> (ø)
include/votca/xtp/qmmolecule.h	100.0% <100.0%> (ø)

---

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 312a8a9...032d9e4. Read the comment docs.

[baumeier]

FWIW, this has fixed the problem. Not sure about the other concerns @JensWehner mentioned.

[rubengerritsen]

At the moment it seems that AddContainer is only used on QMAtoms. So it should be fine.

To make sure that it will be fine in the future as well, I propose we untemplate the AddContainer function and make it work for QMAtom containers only. This is a bit ugly because it breaks the general/templated usage of the AtomContainer, but at the same time it will give a compiler warning if someone tries to use the function with another type. See the last commit for details.

[rubengerritsen]

@votca-bot changelog: fixed atomid numbering while adding containers

[rubengerritsen]

At the moment it seems that AddContainer is only used on QMAtoms. So it should be fine.

To make sure that it will be fine in the future as well, I propose we untemplate the AddContainer function and make it work for QMAtom containers only. This is a bit ugly because it breaks the general/templated usage of the AtomContainer, but at the same time it will give a compiler warning if someone tries to use the function with another type. See the last commit for details.

I might be wrong, I am looking at the iqm.cc now and I don't know how the atomids of the A and B segments behave, I will try to test it out this afternoon

[JensWehner]

The atomids are only needed for atomwise operations, which iqm should not do, as far as I know.
Untemplating the add container is the right call.

[rubengerritsen]

For the calculations that we do now, I agree. But the annoying thing is that the atomIDs of the B segment in iqm will always be offset by the size of the A segment if we merge the current fix in master. If someone in the future ever needs to use the atomIDs in iqm it will be very hard to figure that out.

So I have a new idea, that I will try to implement. The QMregion in qmmm makes in essence a new "molecule" (can be multiple) that is done in the QMRegion::push_back, if we update the atom IDs there then it won't affect any other classes. Furthermore the AddContainer behaves as one should expect, by simply appending one container to another without influencing the content.

[JensWehner]

Yeah yes and no, the atomid inside a molecule should be unique, even if you add two molecules to form a new one. So renumbering is actually a desirable property. For the other molecule like containers, I am not sure if the atomid can also correspond to the atomid inside the molecular dynamics simulation.

[rubengerritsen]

After a QMmapping all atoms are stripped of their original (MD sim) atomIDs and hence all start at zero. My main concern raised above about the iqm was just wrong. Hence the original fix is the real fix.

[JensWehner]

I thought we wanted to move this up to the QMMolecule?

[rubengerritsen]

Hmm, I am not really sure that we understood one another, because that is not necessarily what I was thinking. Though it is a good idea. So hopefully I understood it correctly this time. I have added a new commit in which I moved the AddContainer to QMMolecule.