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

fixes replicafiller #64

Merged
merged 5 commits into from
Mar 18, 2019
Merged

fixes replicafiller #64

merged 5 commits into from
Mar 18, 2019

Conversation

SteffenSeckler
Copy link
Member

fixes replicafiller also when using mpi.

assumes:

  • The ParticleContainerToBasisWrapper does not have to do any cutting of particles
  • Removing particles happens later, i.e., by the ReplicaFiller!

what has been done:

  • src/particleContainer/ParticleContainer.h: make various functions virtual, s.t. it actually is used by the ParticleContainerToBasisWrapper!
  • src/utils/generator/ReplicaFiller.cpp: make the ParticleContainerToBasisWrapper accept all particles, by implementing getBoundingBoxMin, getBoundingBoxMax, isInBoundingBox
  • don't pass any object to the ParticleContainerToBasisWrapper, no check needs to be done here yet!
  • abort if no particles have been added to the ParticleContainerToBasisWrapper

in addition:

  • some clang-tidy fixes

remark:

  • some tidy-up work could still be done inside of the ParticleContainerToBasisWrapper

Copy link

@tchipev tchipev left a comment

Choose a reason for hiding this comment

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

Looks great, I even tested it briefly vs revision 6000 - the numbers are good.
But please indicate that it's a dirty fix.
Make an Issue with recommendations, observations, etc. on the ReplicaFiller class for eventual clean-up.
It seems to me that the whole ParticleContainerToBasisWrapper approach is a bad idea. Perhaps a more abstract interface should be introduced, from which ParticleContainer and whatever this should be can also derive.

src/io/BinaryReader.cpp Outdated Show resolved Hide resolved
src/utils/generator/ReplicaFiller.cpp Show resolved Hide resolved
src/utils/generator/ReplicaFiller.cpp Show resolved Hide resolved
src/utils/generator/ReplicaFiller.cpp Outdated Show resolved Hide resolved
Copy link

@tchipev tchipev left a comment

Choose a reason for hiding this comment

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

OK

@SteffenSeckler SteffenSeckler merged commit dad35cf into master Mar 18, 2019
@SteffenSeckler SteffenSeckler deleted the feature/objectreaderfixes branch March 18, 2019 16:49
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