WIP: Add particles by brryan · Pull Request #1 · brryan/parthenon-2

brryan · 2020-04-11T00:02:38Z

PR Summary

Add particles to parthenon. This PR shows what has changed relative to master

PR Checklist

Code passes cpplint
New features are documented.
Adds a test for any bugs fixed. Adds tests for new features.

Yurlungur

Overall looks pretty cool! I like the design. I include below a number of random comments that came to mind as I skimmed this.

Yurlungur · 2020-04-16T23:33:36Z

-    containers_["base"] = std::make_shared<Container<T>>(); // always add "base" container
+    // Always add "base" containers
+    containers_["base"] = std::make_shared<Container<T>>();
+    swarm_containers_["base"] = std::make_shared<SwarmContainer>();


Hmm... My intuition is to say that the containers should own swarms, rather than a container collection owning a bunch of swarm containers. Is there a reason not to go that route?

The big complication I see is that the particles might move on different timesteps than the particles, in which case, the stages might need to be decoupled. Is that why you went this route?

There's a lot going on in the current swarm hierarchy that I sort of guessed at so this should probably change somehow. I'm not sold on containers owning swarms though, since the container seemed specialized to fields when I looked through the code. Now that I think about it I take your point though.

I think particles and fields evolving with different time integration stages, different timesteps etc., is a good motivation though for keeping swarm containers at the same level as containers ("field containers"?). I hadn't thought of this. Anyway this should probably keep getting brought up for a while.

Perhaps container should be renamed field_container ;)

could also be called not_swarm_container

Yurlungur · 2020-04-16T23:38:23Z

+enum class PARTICLE_TYPE {
+  INT, REAL, STRING
+};


Is there a need for INT and STRING particle types? Can they not all be Real? That would be easier code-wise.

Indices of the cell the particle is currently living in are I think best handled with integer particle values. PARTICLE_TYPE could be misleading (I think this enum is deprecated in favor of new Metadata flags now), this is just the type of one data field for each particle (so one scalar per particle).

Hmm... Fair enough. I would lean on getting rid of strings, though. They will be difficult to translate onto GPU.

Yurlungur · 2020-04-16T23:39:20Z

+enum class PARTICLE_STATUS {
+  UNALLOCATED, ALIVE, DEAD
+};


As we discussed, I think I'd just make dead particles have zero weight. Not sure about unallocated. Maybe this is still useful for a whole swarm?

There'll be some interplay between particles that are dead and regions of the pool that have never been allocated or are beyond the maximum living particle, I think. Some notion of UNALLOCATED may be useful here, but this is very much in flux at the moment.

A negative weight or something could be used to denote that. I admit that might be confusing, though.

Yurlungur · 2020-04-16T23:42:53Z

+  std::vector<std::string> _labelArray;
+  std::vector<PARTICLE_TYPE> _typeArray;
+  std::shared_ptr<ParArrayND<PARTICLE_STATUS>> _pstatus;
+  ParticleVariableVector<int> _intVector;
+  std::vector<std::shared_ptr<ParArrayND<Real>>> _realVector;
+  std::vector<std::shared_ptr<ParArrayND<std::string>>> _stringVector;


Something to note about the ParArrayND design: ParArrayND already has a label. Which may mean that the array of labels isn't really necessary. I'm not sure.

Also as I mentioned above, do you need string and int fields for particles? Is that necessary?

Yes labelArray and typeArray can go away, these are from an earlier iteration and I forgot to delete them.

Yurlungur · 2020-04-16T23:46:05Z

+  if (intMap_.find(label) != intMap_.end() ||
+      realMap_.find(label) != realMap_.end() ||
+      stringMap_.find(label) != stringMap_.end()) {
+    throw std::invalid_argument ("swarm variable " + label + " already enrolled during Add()!");


I think we're moving towards turning all these throws in the interface into more primitive code dying mechanisms. Since exception handling is slow.

I'd be happy to turn these into exits, maybe a FAIL macro that automatically prints __FILE__ and __LINE__? :)

If you wrote such a macro and contributed it upstream, I think there would be interest. :)

Cool... I'm still tempted to call it assertions.hpp though (Draco has an Assert.hh and I never could think of another appropriate name for such a file, maybe you have suggestions)

error_handling.hpp

Ooh perfect

Yurlungur · 2020-04-16T23:47:05Z

+  if (intMap_.find(label) != intMap_.end() ||
+      realMap_.find(label) != realMap_.end() ||
+      stringMap_.find(label) != stringMap_.end()) {


instead of map_.find I might use map.count(label) > 0. I find that to be more intuitive.

Yes I like that better. map_.find is just what came up first in google.

Yurlungur · 2020-04-16T23:49:49Z

+  //Public Methods
+  //-----------------
+  // Constructor does nothing
+  SwarmContainer() {


Suggested change

SwarmContainer() {

SwarmContainer() = default;

Yurlungur · 2020-04-16T23:50:38Z

+  /// Get a swarm from the container
+  /// @param label the name of the swarm
+  /// @return the Swarm if found or throw exception
+  Swarm& Get(std::string label) {


Don't linear search the vector. Use the map.

Oh right, leftover code, good catch :)

Yurlungur · 2020-04-16T23:58:14Z

Oh one more thought. Weight will probably always be wanted, even if it's just 1 or 0. So maybe every swarm should start with a weight field.

brryan · 2020-04-17T01:05:00Z

I was wondering what default fields should be, if any. x, y, and z?. I think I'm in favor of every particle having a default mask field, but as to weight this may be too application dependent, and users may not want their "weight" to actually be called "weight" or "w" or whatever we decree.

Yurlungur · 2020-04-17T17:40:33Z

Hmm a good point. I guess we should lean on zero default fields until we start writing the communication code, etc., and then discover we need some of them.

brryan added 9 commits April 9, 2020 22:17

Added SwarmContainer

8c7a8f4

Added Swarm

e270277

Get code to compile

971a47c

Compiling again

6803d36

Quick fix

fabd811

Merge branch 'master' into particles

e5602e9

Moved swarmcontainer into containercollection

ec8d8ed

Adding example

3547eb5

Still a lot of work to do

6a78b27

Yurlungur reviewed Apr 16, 2020

View reviewed changes

brryan added 3 commits April 17, 2020 12:04

Amazingly, this almost compiles

e761f9a

Amazingly, compiles

c5fd5e0

particles example runs to completion now but I don't know why

22351a1

Conversation

brryan commented Apr 11, 2020

PR Summary

PR Checklist

Uh oh!

Yurlungur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Yurlungur commented Apr 16, 2020

Uh oh!

brryan commented Apr 17, 2020

Uh oh!

Yurlungur commented Apr 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants