Add OpeningAngle to BinaryCompactObject #4934

mar-celine · 2023-04-11T09:00:13Z

Proposed changes

Addresses issue #3545 by introducing an OpeningAngle option to the BinaryCompactObject Domain that allows the user to set the angular sizes of the Wedges in the Domain, making the angular distribution of points more uniform.

Upgrade instructions

Add the new OuterShell.OpeningAngle option to BinaryCompactObject domains in input files. Set it to 90 to keep the previous behavior.

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

The old domain. One can also see how the grid becomes rounded-cubical moving outwards from the two inner cubes. This is left over from bulging out from a cubical shell of frustums (the old enveloping cube), The main concern however, was the fact that the two half wedges have the same angular size as a full wedge, despite having twice the number of gridpoints between them. The proposed solution was to fan out the half wedges and make the end cap full wedges smaller in angular size.

The new domain created from the same options, except with OpeningAngle set to 120.0. The enveloping cube is stretched into a rectangular prism first, which is then bulged out, which seems to give a better gridpoint distribution than before. The Wedges in the outer layer are also opened up to conform to the new bulged out prism, and the equiangular map is revamped to accomodate variable opening angle sizes.

nilsvu · 2023-04-13T01:00:09Z

Looks great! Thanks for your work on this! I'll review asap.

knelli2

Just some minor changes. Love the new image though!

knelli2 · 2023-04-25T16:05:34Z

tests/Unit/Domain/CoordinateMaps/Test_Wedge3D.cpp

+      /*1.0 / sqrt(with_equiangular_map
+                     ? 1.0 + square(tan(0.5 * opening_angle_xi)) +
+                           square(tan(0.5 * opening_angle_eta))
+                     : 3.0);*/


knelli2 · 2023-04-25T16:07:06Z

src/Domain/CoordinateMaps/Frustum.cpp

+  // the Frustum that is furthest away from the origin. For the rectangular
+  // BinaryCompactObject Domain, this vertex is assumed to lie on a rectangular


Is this true for domains other than the BCO domain?

This is a good question - there is only one other Domain that uses Frustums, the FrustalCloak Domain, but this Domain has not been updated or used in a while. I expect the BinaryCompactObject Domain to be the only domain the typical user uses that would lead them to encounter a Frustum map, which is why I added this comment.

I could add, For information on how other Domains use the Frustum map, please see that Domain's documentation. or something to that effect.

Yeah I think that comment would be helpful.

knelli2 · 2023-04-25T16:08:00Z

src/Domain/CoordinateMaps/Frustum.hpp

+      std::numeric_limits<double>::signaling_NaN()};
 };

 bool operator!=(const Frustum& lhs, const Frustum& rhs);


[Random line] Should there be a test for the changes to the Frustum?

I will add a random opening_angle to the test suite for map to check that the map is valid for various opening angles with the correct inverse/jacobian. Additional tests for "does this map do what we want" will be taken care of by the BCO domain's "check connectivity" test that checks that the Frustums with opening angles conform to the Wedges with opening angles.

knelli2 · 2023-04-25T16:18:16Z

src/Domain/Creators/BinaryCompactObject.hpp

+    static constexpr Options::String help = {
+        "The opening angle of the two half wedges in degrees."};


Maybe be a bit more descriptive? I don't think a user necessarily knows what the half wedges are. Maybe something like

Surrounding the two objects (which are encompassed by two cubes) are a series of wedges that abut the faces of these cubes. The wedges on the ends of these cubes are full wedges while the wedges around the rest of the faces are half wedges. This option is the opening angle of these half wedges in degrees.

If I said something incorrect here you can just fix it when you add it.

I don't want to make the help string too long and since this option is placed in group = OuterShell and the outer shell is in the main BCO documentation I can change half wedges to half wedges of the outer shell and the user should have enough information to find out what this is referring to with this string. But I will also add documentation on the half wedges in that place!

Ok yeah that sounds good.

knelli2 · 2023-04-25T16:19:51Z

src/Domain/Creators/BinaryCompactObject.cpp

  // centered at the origin.
+  const double sphericity = 1.0;


constexpr

knelli2 · 2023-04-25T16:21:40Z

tests/Unit/Domain/Creators/Test_BinaryCompactObject.cpp

  constexpr double outer_radius = 32.4;
+  constexpr double opening_angle = 120.0;


Can you instead make this part of the random sample so we can test different values? Maybe make_array(75.0, 90.0, 120.0)?

knelli2 · 2023-04-25T16:23:51Z

tests/Unit/Domain/Creators/Test_BinaryCompactObject.cpp

  constexpr double outer_radius = 32.4;
+  constexpr double opening_angle = 90.0;


Same comment as above. You'll probably need to edit the create_option_string as well to take arbitrary values.

knelli2

Looks good. You can squash

mar-celine · 2023-04-26T22:13:37Z

I've squashed! @nilsvu It's ready for you to review ☺️

src/Domain/CoordinateMaps/Wedge.cpp

nilsvu · 2023-05-05T23:10:34Z

src/Domain/CoordinateMaps/Frustum.cpp

    p | sphericity_;
    p | radius_;
    p | phi_;
+  } if (version >= 1) {


clang-format

src/Domain/CoordinateMaps/Frustum.hpp

nilsvu · 2023-05-05T23:19:05Z

src/Domain/DomainHelpers.hpp

 * \param radial_distribution Select the radial distribution of grid points in
 * the spherical shells.
 * \param which_wedges Select a subset of wedges.
+ * \param opening_angle sets the opening angle of the two half wedges that


Add a sentence that this only has an effect if use_half_wedges is turned on. Also maybe name something like half_wedges_opening_angle to clarify that this number applies to the half wedges? Default to M_PI_4 in that case (and multiply by 2 to get the opening angle of the full wedge). This is an attempt to clarify what "opening angle" means for a full envelope of wedges, other suggestions welcome.

I want to keep opening_angle as the angle for the full wedge (or both halves combined) because this makes it easier to compare with the endcap full wedge which is also given an opening_angle, but I'll had that this parameter has no effect if use_half_wedges is turned off

src/Domain/DomainHelpers.hpp

nilsvu · 2023-05-05T23:35:02Z

src/Domain/Creators/BinaryCompactObject.hpp

    static constexpr Options::String help = {"Radius of the entire domain."};
  };

+  struct OpeningAngle {


Same comment here about clarifying what "opening angle" means for a full envelope of wedges. Maybe HalfWedgesOpeningAngle if you want to retain the ability to choose any value for the angles. Values of 45 and 60 degrees would correspond to the previous behavior and the new equal angular size behavior, respectively (note that in the help string). Alternatively, just a boolean like EqualizeOpeningAngles or something like that could also work. WDYT?

(as mentioned above I think it's easier to be able to compare two half wedges with the end cap wedge using the full opening angle vs the half)

I'm not sure if we want to equalize by default yet, since we are sacrificing equiangular-ity in the endcap wedges (but so far runs have not shown poor behavior in those regions resulting from the change).

I have a guess that a value between 120.0 and 90.0 might be ideal to strike a balance between angular size and equiangular-ity, or it could also be the case that we don't want to equalize angular size in angle but rather in surface area / volume covered by each wedge/half wedge - it is not clear to me that a choice of 120.0 results in the half wedge having the same surface area or volume as an endcap wedge (and this still might not be what gives ideal behavior)

In any case, I think it's too soon to default yet 😅

nilsvu

Couple tiny things, you can squash everything. Please add some upgrade instructions to the PR description.

src/Domain/CoordinateMaps/Wedge.cpp

nilsvu · 2023-05-13T01:40:36Z

src/Domain/CoordinateMaps/Frustum.cpp

    p | sphericity_;
    p | radius_;
    p | phi_;
+  } if (version >= 1) {


src/Domain/CoordinateMaps/Wedge.cpp

nilsvu · 2023-05-13T01:43:55Z

src/Domain/DomainHelpers.hpp

 * \param radial_distribution Select the radial distribution of grid points in
 * the spherical shells.
 * \param which_wedges Select a subset of wedges.
+ * \param opening_angle sets the opening angle of the two half wedges that


nilsvu

I think the relevant part for the upgrade instructions is that people have to add an OpeningAngle option to their input files and should get an idea what that does, and set it to 90 to keep the current behavior. Please update when you get the chance.

nilsvu · 2023-06-19T17:50:22Z

I updated the upgrade instructions.

mar-celine force-pushed the from_cube_to_rectangular_prism branch from 2035f2c to 003d78d Compare April 11, 2023 09:03

mar-celine changed the title ~~From cube to rectangular prism~~ Add OpeningAngle to BinaryCompactObject Apr 11, 2023

knelli2 self-requested a review April 11, 2023 17:10

mar-celine force-pushed the from_cube_to_rectangular_prism branch 2 times, most recently from 152ec1f to a0eb9a6 Compare April 11, 2023 21:42

nilsdeppe requested a review from nilsvu April 11, 2023 22:17

mar-celine force-pushed the from_cube_to_rectangular_prism branch from a0eb9a6 to d1a1e4d Compare April 12, 2023 19:26

mar-celine mentioned this pull request Apr 12, 2023

Improvements for the BinaryCompactObject domain #3545

Closed

3 tasks

geoffrey4444 added this to the Optimize domain for BBH simulations milestone Apr 19, 2023

geoffrey4444 assigned mar-celine Apr 19, 2023

mar-celine force-pushed the from_cube_to_rectangular_prism branch 6 times, most recently from a0a4794 to 990d719 Compare April 25, 2023 01:05

knelli2 requested changes Apr 25, 2023

View reviewed changes

knelli2 reviewed Apr 26, 2023

View reviewed changes

mar-celine force-pushed the from_cube_to_rectangular_prism branch from cb5dc0c to fbd6ae7 Compare April 26, 2023 21:54

mar-celine force-pushed the from_cube_to_rectangular_prism branch from fbd6ae7 to 83eba48 Compare May 3, 2023 21:20

nilsvu requested changes May 5, 2023

View reviewed changes

nilsvu requested changes May 13, 2023

View reviewed changes

mar-celine added 4 commits May 15, 2023 11:24

Add opening_angles to Wedge map

475f7d2

Add opening_angle to Frustum map

874b0b7

Update DomainHelpers to include opening_angle

d2f023d

Add OpeningAngle option to BinaryCompactObject

aa7f387

mar-celine force-pushed the from_cube_to_rectangular_prism branch from 04bd8fa to aa7f387 Compare May 15, 2023 18:25

mar-celine mentioned this pull request May 16, 2023

Enable Logarithmic Distribution in Envelope of BinaryCompactObject #5006

Merged

3 tasks

nilsvu approved these changes May 16, 2023

View reviewed changes

nilsvu reviewed May 16, 2023

View reviewed changes

nilsvu merged commit d9372f9 into sxs-collaboration:develop May 16, 2023

		// the Frustum that is furthest away from the origin. For the rectangular
		// BinaryCompactObject Domain, this vertex is assumed to lie on a rectangular

		static constexpr Options::String help = {
		"The opening angle of the two half wedges in degrees."};

		constexpr double outer_radius = 32.4;
		constexpr double opening_angle = 120.0;

		constexpr double outer_radius = 32.4;
		constexpr double opening_angle = 90.0;

Add OpeningAngle to BinaryCompactObject #4934

Add OpeningAngle to BinaryCompactObject #4934

Uh oh!

Conversation

mar-celine commented Apr 11, 2023 • edited by nilsvu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Upgrade instructions

Code review checklist

Further comments

Uh oh!

nilsvu commented Apr 13, 2023

Uh oh!

knelli2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knelli2 left a comment

Choose a reason for hiding this comment

Uh oh!

mar-celine commented Apr 26, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nilsvu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nilsvu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nilsvu commented Jun 19, 2023

Uh oh!

Reviewers

mar-celine commented Apr 11, 2023 •

edited by nilsvu

Loading

nilsvu left a comment •

edited

Loading

nilsvu left a comment •

edited

Loading