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.

Sign up for GitHub

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

Jump to bottom

Add rotation, expansion and translation maps to Sphere domain #5758

Merged
merged 1 commit into from
Feb 26, 2024
Merged

Add rotation, expansion and translation maps to Sphere domain #5758

merged 1 commit into from
Feb 26, 2024

Conversation

guilara
Copy link
Contributor

@guilara guilara commented Feb 13, 2024 •
edited
Loading

Proposed changes

We add the RotScaleTrans time-dependent maps as options for the Sphere domain

Upgrade instructions

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Further comments

@guilara guilara marked this pull request as draft February 13, 2024 17:06
@nilsvu nilsvu requested a review from knelli2 February 14, 2024 21:43
@guilara guilara changed the title Add new translation map to sphere domain Add rotation, expansion and translation maps to sphere domain Feb 18, 2024
@guilara guilara changed the title Add rotation, expansion and translation maps to sphere domain Add rotation, expansion and translation maps to Sphere domain Feb 18, 2024
@guilara guilara marked this pull request as ready for review February 19, 2024 10:26
knelli2
knelli2 requested changes Feb 19, 2024
View reviewed changes
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.

Thank you for doing this! Please add these changes in a fixup.

src/Domain/Creators/SphereTimeDependentMaps.hpp Outdated Show resolved Hide resolved
src/Domain/Creators/SphereTimeDependentMaps.hpp Outdated Show resolved Hide resolved
src/Domain/Creators/SphereTimeDependentMaps.hpp Outdated Show resolved Hide resolved
src/Domain/Creators/SphereTimeDependentMaps.hpp Outdated Show resolved Hide resolved
src/Domain/Creators/SphereTimeDependentMaps.hpp Outdated Show resolved Hide resolved
src/Domain/Creators/SphereTimeDependentMaps.cpp Outdated Show resolved Hide resolved
src/Domain/Creators/Sphere.cpp Outdated Show resolved Hide resolved
src/Domain/Creators/SphereTimeDependentMaps.hpp Outdated Show resolved Hide resolved
tests/InputFiles/Xcts/KerrSchild.yaml Outdated Show resolved Hide resolved
src/Domain/Creators/SphereTimeDependentMaps.hpp Outdated Show resolved Hide resolved
@guilara guilara requested a review from knelli2 February 21, 2024 15:35
knelli2
knelli2 requested changes Feb 21, 2024
View reviewed changes
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.

Last couple things

src/Domain/Creators/SphereTimeDependentMaps.cpp Outdated Show resolved Hide resolved
src/Domain/Creators/SphereTimeDependentMaps.hpp Outdated Show resolved Hide resolved
@guilara guilara requested a review from knelli2 February 22, 2024 16:00
knelli2
knelli2 reviewed Feb 22, 2024
View reviewed changes
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.

Ok everything looks good! Please squash everything down into one commit. Thank you for doing this!

@guilara
Add translation map to sphere
399ec27 
Add rotation and expansion options

Add test for translation time dependence in Sphere

Fix RotScaleTrans initial values and other fixes

Fix

Fix
@guilara guilara requested a review from knelli2 February 26, 2024 07:18
knelli2
knelli2 approved these changes Feb 26, 2024
View reviewed changes
Copy link
Contributor

@knelli2 knelli2 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for adding this! Ignoring unrelated test failure.

@knelli2 knelli2 merged commit daeff9d into sxs-collaboration:develop Feb 26, 2024
21 of 22 checks passed
@knelli2
Copy link
Contributor

knelli2 commented Feb 26, 2024

@guilara Could you please add UPDATE INSTRUCTIONS in the PR description? This is info on how users need to update their input files to use your new changes

@knelli2 knelli2 mentioned this pull request Mar 19, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nilsvu nilsvu nilsvu left review comments

@knelli2 knelli2 knelli2 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@guilara @knelli2 @nilsvu