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

Use new common time dependent options in Sphere and BCO domains #6115

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

knelli2
Copy link
Contributor

@knelli2 knelli2 commented Jun 25, 2024

Proposed changes

Final PR in the series that revamps the time dependent options for the Sphere and BCO domains.

This actually adds all the new common options to the domain creators.

Fixes #5584.

Upgrade instructions

The options for the Rotation and Expansion map in the BinaryCompactObject and Sphere domains have changed. In the BinaryCompactObject, they are now

    TimeDependentMaps:
      ...
      ExpansionMap:
        InitialValues: [1.0, -4.6148457646200002e-05, 0.0] # <-- Must specify all derivs
        AsymptoticVelocityOuterBoundary: -1.0e-6 # <-- Same as before
        DecayTimescaleOuterBoundary: 50.0 # <-- Same as before
      RotationMap:
        InitialQuaternions: [[1.0, 0.0, 0.0, 0.0]] # <-- Need quat. Can optionally specify derivs
        InitialAngles: # <-- Need angle. Can optionally specify derivs (like angular velocity)
          - [0.0, 0.0, 0.0]
          - [0.0, 0.0, 1.5264577062000000e-02] # <-- This is now angular velocity

In the Sphere domain, you can optionally specify the same options as the BinaryCompactObject like above to use functions of time that expire. Additionally, you can specify options to use SettleToConstant functions of time in the Sphere domain with the options below

    TimeDependentMaps:
      ...
      ExpansionMap:
        InitialValues: [1.0, -4.6148457646200002e-05, 0.0] # <-- Must specify all derivs
        InitialValuesOuterBoundary: [1.0, 0.0, 0.0] # <-- Must specify all derivs
        DecayTimescale: Auto # <-- Used for SettleToConst FoT
        DecayTimescaleOuterBoundary: 50.0
      RotationMap:
        InitialQuaternions: [[1.0, 0.0, 0.0, 0.0]] # <-- Need quat. Can optionally specify derivs
        DecayTimescale: 50 # <-- Used for SettleToConst FoT

Additionally, for any of the time dependent map options, you can choose the function of time from a volume H5 file by specifying the following options (not just for Expansion, but for Rotation and Translation as well and shape is slightly different)

    TimeDependentMaps:
      ...
      ExpansionMap:
        H5Filename: BbhVolume0.h5
        SubfileName: VolumeData
      ShapeMapA:
        H5Filename: BbhVolume0.h5
        SubfileName: VolumeData
        TransitionEndsAtCube: true

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

Depends on and includes #6109, #6113, and #6114.

@knelli2 knelli2 added this to the BBH First Paper milestone Jun 25, 2024
@knelli2 knelli2 changed the title Dc time dep opts 4 Use new common time dependent options in Sphere and BCO domains Jun 25, 2024
@knelli2 knelli2 added dependent Needs a different PR to be merged in first labels Jun 26, 2024
@knelli2 knelli2 removed dependent Needs a different PR to be merged in first labels Sep 19, 2024
@knelli2 knelli2 added the in progress Don't review, used for sharing code and getting feedback label Sep 23, 2024
@knelli2 knelli2 force-pushed the dc_time_dep_opts_4 branch 2 times, most recently from a6699f1 to d82e058 Compare December 18, 2024 22:38
Copy link
Member

@nilsvu nilsvu left a comment

Choose a reason for hiding this comment

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

ok I'll need more coffee for this one

You can squash everything into one commit, except keep the "Allow FoTs to create themselves" commit separate.

Comment on lines 76 to 79
std::array<DataVector, MaxDeriv + 1> all_derivs{};
for (size_t i = 0; i < all_derivs.size(); i++) {
gsl::at(all_derivs, i) = std::move(all_derivs_vec[i]);
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this is make_array<MaxDeriv+1>(func_and_all_derivs(t))

Comment on lines 87 to 90
std::array<DataVector, MaxDeriv + 1> all_angle_derivs{};
for (size_t i = 0; i < all_angle_derivs.size(); i++) {
gsl::at(all_angle_derivs, i) = std::move(all_angle_derivs_vec[i]);
}
Copy link
Member

Choose a reason for hiding this comment

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

also make_array

struct TranslationMapOptions {
using type = Options::Auto<TranslationMapOptions, Options::AutoLabel::None>;
static std::string name() { return "TranslationMap"; }
struct TranslationMapHardcodedOptions {
Copy link
Member

Choose a reason for hiding this comment

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

I always read "hardcore options" 💪

Suggestion: keep this class named TranslationMapOptions. And the tag below name just TranslationMap because it's already in the time_dependent_options namespace.

Also this moves the FromVolumeFile option up a level in options
so all the time dependent options are edited as well
@knelli2 knelli2 removed the in progress Don't review, used for sharing code and getting feedback label Dec 19, 2024
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.

Read in FoT parameters from file in BCO domain creators
2 participants