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

Unify derivatives between CarpetX and SpacetimeX. #2

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

stevenrbrandt
Copy link
Contributor

(1) Discard existing derivs.hxx and derivs.cxx
(2) Merge z4c and weyl derivs.hxx
(3) move epsdiss to Derivs

(1) Discard existing derivs.hxx and derivs.cxx
(2) Merge z4c and weyl derivs.hxx
(3) move epsdiss to Derivs
@lwJi lwJi requested a review from eschnett May 25, 2023 21:18
@@ -695,7 +695,8 @@ Numerical/AEILocalInterp
# CarpetX thorns
!TARGET = $ARR
!TYPE = git
!URL = https://github.com/eschnett/CarpetX.git
Copy link
Contributor

Choose a reason for hiding this comment

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

This is convenient for testing, but this shouldn't be merged.

@@ -15,6 +15,10 @@ USES INCLUDE HEADER: ten3.hxx
USES INCLUDE HEADER: vec.hxx
USES INCLUDE HEADER: vect.hxx

INHERITS: Derivs
Copy link
Contributor

Choose a reason for hiding this comment

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

INHERITS is for grid functions, but we don't need any grid functions from Derivs.

To ensure that the derivative operators are actually present, thorn Derivs needs to provide a capability Derivs, and thorns Weyl and Z4c need to requires that derivative. Both are handled in the respective configuration.ccl files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already done.

@@ -695,7 +695,8 @@ Numerical/AEILocalInterp
# CarpetX thorns
!TARGET = $ARR
!TYPE = git
!URL = https://github.com/eschnett/CarpetX.git
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 split this into three pull requests instead of two?

  1. Add new capabilities (and tests) to thorn Derivs
  2. Switch over thorns Weyl and Z4c
  3. Clean up CarpetX
    This will allow the changes to be tested and reviewed independently.

@lwJi lwJi requested review from rhaas80 and removed request for rhaas80 May 27, 2023 23:19
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