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

Add iterative scheme to worldtube #5855

Merged
merged 4 commits into from
May 7, 2024

Conversation

nikwit
Copy link
Contributor

@nikwit nikwit commented Mar 16, 2024

Adds the iterative scheme to the worldtube, as described in section V A of https://arxiv.org/abs/2403.08864.

The worldtube sends the updated acceleration to the adjacent elements which use it to compute the updated puncture field and then the updated regular field. This is send back to the worldtube and used to update the acceleration which can be repeated an arbitrary number of times. An action test of this iterative scheme is included.

depends on #5847

@nikwit nikwit changed the title Iterations 2 Add iterative scheme to worldtube Mar 16, 2024
@nikwit nikwit added the dependent Needs a different PR to be merged in first label Mar 16, 2024
@nikwit nikwit removed the dependent Needs a different PR to be merged in first label Apr 26, 2024
@nikwit nikwit requested a review from knelli2 April 26, 2024 09:39
@nikwit nikwit force-pushed the iterations-2 branch 2 times, most recently from b265111 to b669469 Compare April 26, 2024 12:59
Copy link
Contributor

@knelli2 knelli2 left a comment

Choose a reason for hiding this comment

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

Also please fix the clang-tidy errors

Comment on lines +165 to +168
if (db::get<Tags::CurrentIteration>(box) <
db::get<Tags::MaxIterations>(box) - 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this logic just go in IteratePunctureField instead? I think it makes more sense for IteratePunctureField to be the one to increase the current_iteration and determine if it actually needs to compute another iteration of the puncture field or go to BCs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I had originally but I think this way is better because it automatically handles the cases where MaxIterations is equal to 0 (geodesic) or 1 and the IteratePunctureField and IterateAccelerationTerms actions are skipped entirely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh I see. I guess I was a bit confused because checking the iterations didn't seem related to sending data to the worldtube. But the beginning of the iterate actions can determine if an iteration needs to happen or not. I don't have a strong opinion on this so I'll defer to you.

@nikwit nikwit requested a review from knelli2 May 2, 2024 16:59
Copy link
Contributor

@knelli2 knelli2 left a comment

Choose a reason for hiding this comment

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

Squash

Comment on lines +165 to +168
if (db::get<Tags::CurrentIteration>(box) <
db::get<Tags::MaxIterations>(box) - 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh I see. I guess I was a bit confused because checking the iterations didn't seem related to sending data to the worldtube. But the beginning of the iterate actions can determine if an iteration needs to happen or not. I don't have a strong opinion on this so I'll defer to you.

@nikwit
Copy link
Contributor Author

nikwit commented May 6, 2024

@knelli2 thanks, I decided to leave the iteration decision as is. I noticed that I forgot to actually use the iterated puncture field which I addressed in another fixup commit if you would like to take a look.

Copy link
Contributor

@knelli2 knelli2 left a comment

Choose a reason for hiding this comment

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

Looks good. There are a couple more clang-tidy errors, so you can squash those in as well.

@knelli2
Copy link
Contributor

knelli2 commented May 7, 2024

Ignoring unrelated test failure

@knelli2 knelli2 merged commit 2b1dc02 into sxs-collaboration:develop May 7, 2024
21 of 22 checks passed
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.

2 participants