-
Notifications
You must be signed in to change notification settings - Fork 192
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
Add test to evolve packet on Kerr background #6297
base: develop
Are you sure you want to change the base?
Add test to evolve packet on Kerr background #6297
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just add a few comments I think.
} | ||
} | ||
return x; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is from my own code but... could we add a comment here of the type
// We setup the inertial coordinates such that the element used for the evolution is centered on (6,0,0), and the jacobian matrix is the identity matrix
|
||
std::vector<Particles::MonteCarlo::Packet> packets{packet}; | ||
Particles::MonteCarlo::TemplatedLocalFunctions<2, 2> MonteCarloStruct; | ||
// Getting N and CFL constant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "Getting N" is not needed here.
return x; | ||
} | ||
|
||
void test_evolve_kerr(const size_t mesh_size_1d) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add here a comment of the type
// Evolve a single Monte-Carlo packet along a geodesic of Kerr, and check that final position matches expectations.
611df7d
to
f6ef60f
Compare
@nilsdeppe @kidder I reviewed this, not sure what is the process for others to have a look (I'm not a maintainer so the tests won't run just based on my review) |
@SamanthaRath Please look at the output of the test and fix as appropriate. For some of this, you'll want to compile your code in debug mode (some errors are just missing const and type conversions, but there is an array access out of bounds, which likely comes from some tensors being defined on the mesh without ghost zone when they need ghost zones) |
Note that this replaces #6008 |
504a885
to
f9e1d1e
Compare
@kidder @nilsdeppe Could we run the test on this? Samantha uploaded changes that should handle the errors we got after rebasing against the new ghost zone code. |
Okay, I've never seen this message before |
Alright, I'll ignore it then. I don't see why we shouldn't run the test... |
variable types rectified with const Also add ghost zones to MC evolution in Kerr test
f9e1d1e
to
b0d8e21
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good for me now!
@nilsdeppe @kidder Should be ready for someone else to look at it if desired. |
@nilsdeppe @kidder Ping on this (I reviewed the PR, but probably shouldn't commit... especially as I wrote part of the submitted code...) |
Proposed changes
Upgrade instructions
Code review checklist
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.
bugfix
ornew feature
if appropriate.Further comments