Enable equiangular maps in BBH domain #4676

nilsvu · 2023-01-31T22:13:15Z

Proposed changes

Another step towards #3545.

Upgrade instructions

In input files using the BinaryCompactObject domain creator, add the UseEquiangularMap: True option.

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

mar-celine

Looks good! Just some minor comments

mar-celine · 2023-02-01T18:18:10Z

src/Domain/DomainHelpers.cpp

@@ -717,7 +717,8 @@ std::vector<domain::CoordinateMaps::Frustum> frustum_coordinate_maps(
        use_equiangular_map,
        projective_scale_factor,
        false,
-        sphericity});
+        sphericity,
+        -1.});


Can have be -1.0 and 1.0 on line 736 for consistency with the rest of the file. Also for clarity for the last argument (since the other arguments have variable names) can add a comment on line 702 (The frustums on the left are given a -1.0 indicating a lower half equiangular map while the frustums on the right are given a 1.0 indicating an upper half equiangular map) (Ideally this should probably be an enum, but that was my own mistake!)

mar-celine · 2023-02-01T18:18:22Z

src/Domain/Creators/BinaryCompactObject.hpp

@@ -589,7 +596,8 @@ class BinaryCompactObject : public DomainCreator<3> {
      double outer_radius_domain,
      const typename InitialRefinement::type& initial_refinement,
      const typename InitialGridPoints::type& initial_number_of_grid_points,
-      bool use_projective_map = true, double frustum_sphericity = 0.0,
+      bool use_equiangular_map = true, bool use_projective_map = true,
+      double frustum_sphericity = 0.0,


Minor comment here and for line 626: Now that we have equiangular capability, and the frustum layer can be made spherical, should this default value for the frustum_sphericity be kept at 0.0? Maybe it can be changed to be 1.0 now unless there is a good reason to avoid using that value by default.

mar-celine · 2023-02-01T18:19:50Z

src/Utilities/CartesianProduct.hpp

@@ -0,0 +1,64 @@
+// Distributed under the MIT License.


This would be good as a separate commit because it is conceptually unrelated to the domain changes.

It is a separate commit :)

mar-celine

Looks good to me!

nilsdeppe

You can squash and rebase immediately

nilsdeppe · 2023-02-03T16:56:14Z

src/Utilities/CartesianProduct.hpp

+
+}  // namespace detail
+
+// convert a tuple of ranges to the range of tuples representing the Cartesian


Why no Doxygen comment here?

nilsdeppe · 2023-02-03T16:57:17Z

src/Utilities/CartesianProduct.hpp

+  op();
+}
+
+// "peal off" the first range from the remaining tuple of ranges


peal -> peel, same below

nilsvu force-pushed the equiangular_frustums branch from 3201703 to 4f426c1 Compare January 31, 2023 23:02

nilsvu added this to the Optimize domain for BBH simulations milestone Jan 31, 2023

geoffrey4444 requested a review from mar-celine February 1, 2023 01:03

mar-celine reviewed Feb 1, 2023

View reviewed changes

nilsvu force-pushed the equiangular_frustums branch from 4f426c1 to a8c53a9 Compare February 1, 2023 20:30

nilsvu requested a review from mar-celine February 1, 2023 21:09

mar-celine previously approved these changes Feb 1, 2023

View reviewed changes

geoffrey4444 requested a review from nilsdeppe February 2, 2023 02:46

nilsdeppe requested changes Feb 3, 2023

View reviewed changes

nilsvu added 3 commits February 3, 2023 22:48

Add cartesian_product utility

b082bd6

Enable equiangular maps in BBH domain

8a302a3

Default to spherical frustums in BBH domain

3487321

nilsvu dismissed mar-celine’s stale review via 3487321 February 4, 2023 06:49

nilsvu force-pushed the equiangular_frustums branch from a8c53a9 to 3487321 Compare February 4, 2023 06:49

nilsvu requested a review from nilsdeppe February 5, 2023 19:55

nilsdeppe approved these changes Feb 6, 2023

View reviewed changes

nilsdeppe merged commit 8ac60c0 into sxs-collaboration:develop Feb 6, 2023

nilsvu deleted the equiangular_frustums branch February 6, 2023 08:01

nilsvu mentioned this pull request Feb 6, 2023

Improvements for the BinaryCompactObject domain #3545

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable equiangular maps in BBH domain #4676

Enable equiangular maps in BBH domain #4676

nilsvu commented Jan 31, 2023

mar-celine left a comment

mar-celine Feb 1, 2023

mar-celine Feb 1, 2023 •

edited

mar-celine Feb 1, 2023

nilsvu Feb 1, 2023

mar-celine left a comment

nilsdeppe left a comment

nilsdeppe Feb 3, 2023

nilsdeppe Feb 3, 2023


		} // namespace detail

		// convert a tuple of ranges to the range of tuples representing the Cartesian

Enable equiangular maps in BBH domain #4676

Enable equiangular maps in BBH domain #4676

Conversation

nilsvu commented Jan 31, 2023

Proposed changes

Upgrade instructions

Code review checklist

Further comments

mar-celine left a comment

Choose a reason for hiding this comment

mar-celine Feb 1, 2023

Choose a reason for hiding this comment

mar-celine Feb 1, 2023 • edited

Choose a reason for hiding this comment

mar-celine Feb 1, 2023

Choose a reason for hiding this comment

nilsvu Feb 1, 2023

Choose a reason for hiding this comment

mar-celine left a comment

Choose a reason for hiding this comment

nilsdeppe left a comment

Choose a reason for hiding this comment

nilsdeppe Feb 3, 2023

Choose a reason for hiding this comment

nilsdeppe Feb 3, 2023

Choose a reason for hiding this comment

mar-celine Feb 1, 2023 •

edited