FindHorizon.py: also compute horizon quantities #6024

nilsvu · 2024-05-24T00:08:38Z

Proposed changes

This allows to get the horizon area, mass, spin, etc from BBH initial data just by calling a Python function.

Upgrade instructions

The FindHorizons3D executable was removed. You can find horizons in 3D volume data in Python like this:

from spectre.Pipelines.Bbh.FindHorizon import find_horizon
from spectre.SphericalHarmonics import Strahlkorper

horizon, quantities = find_horizon(
    h5_files, subfile_name, obs_id, obs_time,
    initial_guess=Strahlkorper(l_max, m_max, radius, center)
)

mass = quantities["ChristodoulouMass"]
spin = quantities["DimensionlessSpinMagnitude"]

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

knelli2

Add in the upgrade instructions that the FindHorizon exec was removed and that the preferred method for finding horizons in data on disk is the CLI now

src/PointwiseFunctions/GeneralRelativity/Surfaces/Python/Bindings.cpp

knelli2 · 2024-05-25T19:57:42Z

src/PointwiseFunctions/GeneralRelativity/Surfaces/Python/Bindings.cpp

+         tnsr::ii<DataVector, Dim, Frame> spatial_metric,
+         tnsr::II<DataVector, Dim, Frame> inv_spatial_metric,


What about only requiring the inv_spatial_metric and just compute the determinant_and_inverse here? That way less data needs to be output

The spatial metric is in the output anyway because we load it as initial data. I could drop the inv_spatial_metric from the volume data and compute it here. But the horizon finder needs the inv_spatial_metric, so it would have to compute the inverse in every iteration of the horizon finder. Not sure what's best, so I think just keep as is for now?

I'm just worried we are making assumptions about what will be available when we don't necessarily know. Yes for the ID solve we'll have the spatial metric, but what about for a BNS run where somebody wants to see if a horizon formed? Or a supernova bounce? What about taking both as optionals? Then if both are available, you can just use both, but if one or the other are specified then you'll have to compute the inverse (you'd be doing it in volume data anyways)

Can we worry about these applications later? You can always use spectre transform-vol to compute the inverse and other quantities. I could do the optionals but that adds quite a lot of logic that I'm not sure is needed and I think is premature optimization.

Yeah we can worry about them later. I do think people will want to have the option though.

Yes, we want to find horizons in evolution output data. Note that we also have to output ExtrinsicCurvature, spatial Christoffels, and the spatial Ricci tensor, which we probably don't do atm. So we either have to output those (lots of data), or postprocess with transform-vol (possible, maybe best solution and works already, see #6071), or build the postprocessing into the horizon finder routines (possible, except for numerical derivatives), or have more flexible horizon finder triggers in the evolution (hopefully enabled by interpolation framework changes, but doesn't help if you already have the simulation output).

knelli2 · 2024-05-25T20:02:44Z

src/PointwiseFunctions/AnalyticSolutions/GeneralRelativity/Python/Bindings.cpp

+                                            requested_quantity +
+                                            "' is not available.");


Print the available quantities.

Ping on this.

knelli2 · 2024-05-30T19:08:23Z

src/PointwiseFunctions/GeneralRelativity/Surfaces/Python/Bindings.cpp

+         tnsr::ii<DataVector, Dim, Frame> spatial_metric,
+         tnsr::II<DataVector, Dim, Frame> inv_spatial_metric,


Yeah we can worry about them later. I do think people will want to have the option though.

knelli2 · 2024-05-30T19:09:32Z

src/PointwiseFunctions/AnalyticSolutions/GeneralRelativity/Python/Bindings.cpp

+                                            requested_quantity +
+                                            "' is not available.");


Ping on this.

knelli2 · 2024-05-30T19:11:51Z

support/Pipelines/Bbh/FindHorizon.py

    if output:
        assert output_coeffs_subfile or output_coords_subfile, (


Can you add an .h5 extension to the output if there isn't one already? Ran into this today.

knelli2 · 2024-05-31T01:38:03Z

src/ParallelAlgorithms/ApparentHorizonFinder/HorizonAliases.hpp

+    gr::surfaces::Tags::DimensionlessSpinMagnitudeCompute<Frame>,
+    gr::surfaces::Tags::DimensionlessSpinVectorCompute<Frame, Frame>>;


Damn. This was my bad. I forgot that this tag list is used in the ObserveTimeSeriesOnSurface callback, and that requires the types of all tags it observes to be doubles. I roughly know how to generalize this, but haven't had a chance to implement it. So you can't add the compute tag here, but you can just append it to the list in the pybinding cpp.

knelli2

Fixups look good. Squash

nilsvu added the cli/pybindings Command line interface & Python bindings label May 24, 2024

nilsvu added this to the BBH Pipeline Automation milestone May 24, 2024

nilsvu mentioned this pull request May 24, 2024

Automate choice of some params in BBH pipeline #5957

Merged

3 tasks

nilsvu force-pushed the id_horizon_quantities branch 2 times, most recently from dd08192 to 2e4234f Compare May 24, 2024 19:03

nilsvu changed the title ~~FindHorizons.py: also compute horizon quantities~~ FindHorizon.py: also compute horizon quantities May 24, 2024

knelli2 requested changes May 25, 2024

View reviewed changes

nilsvu force-pushed the id_horizon_quantities branch 2 times, most recently from 3ccc9a1 to 41913a7 Compare May 29, 2024 02:03

knelli2 requested changes May 30, 2024

View reviewed changes

knelli2 requested changes May 31, 2024

View reviewed changes

nilsvu force-pushed the id_horizon_quantities branch 2 times, most recently from 7e51107 to 9978ee4 Compare June 3, 2024 19:41

nilsvu requested a review from knelli2 June 5, 2024 21:41

knelli2 approved these changes Jun 6, 2024

View reviewed changes

nilsvu added 7 commits June 6, 2024 16:01

Output spatial Ricci tensor in initial data

ca9f427

FindHorizon.py: also compute horizon quantities

12c230e

Bind KerrSchild solution in Python

7c80b38

Test FindHorizon.py with Kerr solution

9bda502

Delete obsolete FindHorizons executable

ef306dd

Add dimensionless spin vector to horizon observation

809f426

FindHorizon.py: append .h5 to output file name

698b16a

nilsvu force-pushed the id_horizon_quantities branch from 9978ee4 to 698b16a Compare June 6, 2024 23:01

nilsvu merged commit 68c226b into sxs-collaboration:develop Jun 7, 2024
22 checks passed

nilsvu deleted the id_horizon_quantities branch June 18, 2024 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FindHorizon.py: also compute horizon quantities #6024

FindHorizon.py: also compute horizon quantities #6024

nilsvu commented May 24, 2024 •

edited

Loading

knelli2 left a comment

knelli2 May 25, 2024

nilsvu May 29, 2024 •

edited

Loading

knelli2 May 29, 2024

nilsvu May 30, 2024

knelli2 May 30, 2024

nilsvu May 30, 2024 •

edited

Loading

knelli2 May 25, 2024

knelli2 May 30, 2024

knelli2 May 30, 2024

knelli2 May 30, 2024

knelli2 May 30, 2024

knelli2 May 31, 2024

knelli2 left a comment

		tnsr::ii<DataVector, Dim, Frame> spatial_metric,
		tnsr::II<DataVector, Dim, Frame> inv_spatial_metric,

		if output:
		assert output_coeffs_subfile or output_coords_subfile, (

		gr::surfaces::Tags::DimensionlessSpinMagnitudeCompute<Frame>,
		gr::surfaces::Tags::DimensionlessSpinVectorCompute<Frame, Frame>>;

FindHorizon.py: also compute horizon quantities #6024

FindHorizon.py: also compute horizon quantities #6024

Conversation

nilsvu commented May 24, 2024 • edited Loading

Proposed changes

Upgrade instructions

Code review checklist

Further comments

knelli2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsvu May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsvu May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knelli2 left a comment

Choose a reason for hiding this comment

nilsvu commented May 24, 2024 •

edited

Loading

nilsvu May 29, 2024 •

edited

Loading

nilsvu May 30, 2024 •

edited

Loading