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

Apply radiative boundary conditions to Z4c variables #19

Merged
merged 21 commits into from
Nov 28, 2023

Conversation

jaykalinani
Copy link
Contributor

@jaykalinani jaykalinani commented Oct 20, 2023

Apply radiative boundary conditions to Z4c variables using the new NewRadX thorn.

@jaykalinani jaykalinani requested review from lwJi and eschnett October 20, 2023 16:06
Z4c/interface.ccl Show resolved Hide resolved
@@ -12,8 +12,6 @@ BOOLEAN calc_constraints "Calculate constraints" STEERABLE=recover
{
} yes


Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you deleting these lines? They separate two groups of parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -13,7 +13,7 @@ USES INCLUDE HEADER: simd.hxx
USES INCLUDE HEADER: sum.hxx
USES INCLUDE HEADER: vec.hxx
USES INCLUDE HEADER: vect.hxx

USES INCLUDE HEADER: newradx.hxx

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add another empty line here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. However, the following comment suggests arranging the header files alphabetically. For now, I have added an empty line. Should I rather arrange everything alphabetically and remove the empty line?

Z4c/schedule.ccl Outdated Show resolved Hide resolved
#include "loop_device.hxx"
#include "newradx.hxx"

using namespace Loop;
Copy link
Contributor

Choose a reason for hiding this comment

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

You are not using Loop nor std in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Z4c/src/apply_newrad.cxx Outdated Show resolved Hide resolved
Z4c/param.ccl Outdated Show resolved Hide resolved
TwoPuncturesX/schedule.ccl Outdated Show resolved Hide resolved
@@ -13,7 +13,7 @@ USES INCLUDE HEADER: simd.hxx
USES INCLUDE HEADER: sum.hxx
USES INCLUDE HEADER: vec.hxx
USES INCLUDE HEADER: vect.hxx

USES INCLUDE HEADER: newradx.hxx
Copy link
Contributor

Choose a reason for hiding this comment

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

The list of include files should be sorted alphabetically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The above comment suggested instead to add another line instead. Let me know if you prefer an additional line or all header files sorted alphabetically.

Z4c/src/apply_newrad.cxx Outdated Show resolved Hide resolved
@eschnett
Copy link
Contributor

@jaykalinani Can you also add a test case for your changes? Remember that test cases should be small and fast. We don't need to do a convergence test – just a quick setup with a small grid running for 3 iterations would suffice.

Z4c/param.ccl Outdated Show resolved Hide resolved
Z4c/src/apply_newrad.cxx Outdated Show resolved Hide resolved
@jaykalinani
Copy link
Contributor Author

@jaykalinani Can you also add a test case for your changes? Remember that test cases should be small and fast. We don't need to do a convergence test – just a quick setup with a small grid running for 3 iterations would suffice.

Added a test case based on qc0 run.

@lwJi lwJi requested review from rhaas80 and removed request for lwJi November 20, 2023 17:20
@lwJi lwJi changed the title Apply radiative boundary conditions to Z4c variables. Update schedule of TwoPuncturesX Apply radiative boundary conditions to Z4c variables. Nov 23, 2023
@lwJi lwJi changed the title Apply radiative boundary conditions to Z4c variables. Apply radiative boundary conditions to Z4c variables Nov 23, 2023
NewRadX/configuration.ccl Outdated Show resolved Hide resolved
Z4c/param.ccl Show resolved Hide resolved
Z4c/schedule.ccl Show resolved Hide resolved
Z4c/src/apply_newrad.cxx Outdated Show resolved Hide resolved
Z4c/src/apply_newrad.cxx Outdated Show resolved Hide resolved
Z4c/src/apply_newrad.cxx Outdated Show resolved Hide resolved
Z4c/test/test.ccl Outdated Show resolved Hide resolved
@lwJi lwJi merged commit 71eb243 into EinsteinToolkit:main Nov 28, 2023
6 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.

5 participants