Generalize RotationalPeriodicBC for X-, Y-, or Z-axis#3591
Conversation
f1d71b5 to
dbd1a42
Compare
|
I think this is getting close. I'm currently running into a problem when trying to generate the test files I think I understand conceptually what's going on, i.e. the particle is moved to the other periodic surface and continues in the same direction (to where there is no geometry) when it should continue in the opposite direction (inside the geometry). I'm trying to find the correct line to fix this, but I didn't think I changed this behavior from how it worked when there was only rotational periodicity around the z-axis. |
| Position new_r = {cos_theta * r[axis_1_idx_] - sin_theta * r[axis_2_idx_], | ||
| sin_theta * r[axis_1_idx_] + cos_theta * r[axis_2_idx_], r[zero_axis_idx]}; | ||
| Direction new_u = {cos_theta * u[axis_1_idx_] - sin_theta * u[axis_2_idx_], | ||
| sin_theta * u[axis_1_idx_] + cos_theta * u[axis_2_idx_], u[zero_axis_idx]}; |
There was a problem hiding this comment.
Just realized that my "generalization" was based on the z-case that existed here before. The x and z cases should be similar but the y rotation matrix has the sign flipped, which I think explains why I'm seeing one test failure in this way. If we condition on zero_axis_idx I think I can get it right
|
So I just pushed my attempt at accounting for the signs in the transformation of r->new_r and u->new_u, but I'm still failing the tests. I did some scaffolding and it seems like my transformations are not actually transforming anything at all and I'm confused why. For example, the Am I missing some C++ thing about scope or something like that? // rotations around the y-axis have sign flipped for the sin_theta terms
int flip;
if (zero_axis_idx_ == 1) {
flip = -1;
} else {
flip = 1;
}
Position new_r;
new_r[zero_axis_idx_] = r[zero_axis_idx_];
new_r[axis_1_idx_] =
cos_theta * r[axis_1_idx_] - flip * sin_theta * r[axis_2_idx_];
new_r[axis_2_idx_] =
flip * sin_theta * r[axis_1_idx_] + cos_theta * r[axis_2_idx_];
Direction new_u;
new_u[zero_axis_idx_] = u[zero_axis_idx_];
new_u[axis_1_idx_] =
cos_theta * u[axis_1_idx_] - flip * sin_theta * u[axis_2_idx_];
new_u[axis_2_idx_] =
flip * sin_theta * u[axis_1_idx_] + cos_theta * u[axis_2_idx_];EDIT: The above problem derived from the fact that we didn't actually apply the x or y cases in the constructor and so we need to add detection from the two periodic planes to determine which case we are (x,y, or z periodic). Essentially, it was computing a periodic angle of 2pi in the above case |
… they should generate the same answers, need to run transport to get the results_true.dat
…and new_u for correctness
9febb99 to
17ec3eb
Compare
17ec3eb to
c848812
Compare
|
I'm now passing the I did some printing of details, and it looks like the rotation angle currently being detected is 60 degrees (1.0472 radians). When I do the rotation matrix by hand, I'm thinking this is another consequence of the y-rotation matrix / the orientation of the axes and how it corresponds with our diagrams. See below |
…e a PeriodicAxis y
| break; | ||
| case y: | ||
| zero_axis_idx_ = 1; // y component of plane must be zero | ||
| axis_1_idx_ = 2; // z component independent |
There was a problem hiding this comment.
I think you can simplify this PR significantly by switching axis_1_idx , axis_2_idx here.
This switch should make every special case because of y superfluous.
There was a problem hiding this comment.
I get what you mean, i.e. if we switch x and z, then it will behave like the others as long as we're careful about everything. I think it's a little less readable that way and would need a lengthy comment to explain. I kind of like it as is, though. It makes the most physical sense to me, and the math works that way (the y being a special case in the rotation matrix and the compute angle). I'm curious if others have feelings about leaving it or using your clever trick
There was a problem hiding this comment.
Actually, I think you're probably right that switching "xz" to "zx" makes everything better. The fact that all angles will be a -angle rotation makes the rotation matrix the same between all three cases and the compute angle function then doesn't need the special negative. I'll get working on this and push likely today
|
I think I hit the docs everywhere that periodic surfaces / BCs were mentioned and removed past language about limitations this PR resolves. Would love for someone to review that though, and let me know if I missed anywhere and should add more info. |
Co-authored-by: GuySten <62616591+GuySten@users.noreply.github.com>
79f6d94 to
6ecffe6
Compare
GuySten
left a comment
GuySten
left a comment
There was a problem hiding this comment.
Nice work, @lewisgross1296!
GuySten
added
the
Merging Soon
Co-authored-by: GuySten <62616591+GuySten@users.noreply.github.com>
Co-authored-by: GuySten <62616591+GuySten@users.noreply.github.com>
2 participants
Description
Currently only periodicity around the Z-axis is supported. This PR extends this capability for X and Y periodicity. Fixes #3559.
Checklist