Conversation
msaffarini90
left a comment
msaffarini90
left a comment
There was a problem hiding this comment.
I have added few comments, which most are clarifications rather than anything else.
Couple of things that are not clear regarding "BodyTerm", and I added my questions at the corresponding location.
In general, I am going to submit this review as "Request Changes" instead of "Comment" in case my questions inspired any change. However, I don't find any showstoppers or anything that prevents the example from running properly.
| @@ -0,0 +1,20 @@ | |||
| { | |||
| "dx" : {"value": [0.0025, 0.0025, 0.01], "unit": "m"}, | |||
There was a problem hiding this comment.
I am not sure if this is used somewhere else, but I don't see it in any of the added or updated files.
| @@ -0,0 +1,20 @@ | |||
| { | |||
| "dx" : {"value": [0.0025, 0.0025, 0.01], "unit": "m"}, | |||
| "system_size" : {"value": [0.25, 0.25, 1.0], "unit": "m"}, | |||
There was a problem hiding this comment.
I am not sure if this is used somewhere else, but I don't see it in any of the added or updated files.
| { | ||
| }; | ||
| template <> | ||
| struct is_particle_init<Cabana::InitUniform> : public std::true_type |
There was a problem hiding this comment.
Is this added in advance in case they are needed in the future? I don't see them used in the new additions.
There was a problem hiding this comment.
No, this is used in the Particles class. On rereading, it isn't necessary here and could be separated into a new PR
There was a problem hiding this comment.
Could be separated, but directly supports the needs here and will need to match #256 at any rate
| double vz = v( p, 2 ); | ||
| double rz = y( p, 2 ); | ||
| double vn = vz * rz; | ||
| vn /= rz; |
There was a problem hiding this comment.
Isn't this vz? You are multiplying vz by rz on line 127, and then divide by rz on line 128, meaning that
vn = ( vz * rz ) / rz
= vz
?
There was a problem hiding this comment.
You are correct. Ideally this would be a more general wall boundary, but it's pretty simplistic
streeve
left a comment
streeve
left a comment
There was a problem hiding this comment.
Thanks for the review; most important is does the problem run well for you?
| { | ||
| }; | ||
| template <> | ||
| struct is_particle_init<Cabana::InitUniform> : public std::true_type |
There was a problem hiding this comment.
No, this is used in the Particles class. On rereading, it isn't necessary here and could be separated into a new PR
| double vz = v( p, 2 ); | ||
| double rz = y( p, 2 ); | ||
| double vn = vz * rz; | ||
| vn /= rz; |
There was a problem hiding this comment.
You are correct. Ideally this would be a more general wall boundary, but it's pretty simplistic
- Minor formatting
3 participants
Initial case for AoR