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

New fits NRSur3dq8BMSRemnant #31

Open
wants to merge 36 commits into
base: master
Choose a base branch
from
Open

Conversation

gda98re
Copy link

@gda98re gda98re commented Jan 23, 2025

Added the model NRSur3dq8BMSRemnant to the package. This model predicts the proper supertransation modes up to ell = 8 and the 3 components of the boost velocity of the BMS transformation from the inspiral (PN) BMS frame to the remnant black hole BMS frame. The model was trained on nonprecessing quasi-circular binary black hole systems. The fits are done using Gaussian Process Regression (GPR) and also provide an error estimate along with the fit value.

This model has been trained in the parameter space:
q <= 8, |chiAz| <= 0.8, |chiBz| <= 0.8

gdare added 27 commits December 21, 2024 09:38
…d the description of all method in surfinBH.py
… chif and chif_err are None, needed for NRsur3dq8BMSRemnant
…use generate_regression_data.py does not work on these fits yet
…8BMS from the models to loop over in test_interface.py
@duetosymmetry
Copy link
Collaborator

We love to see a net negative 45 line commit

@duetosymmetry
Copy link
Collaborator

Tests are failing because spherical_functions is not a dependency. The only functions used from spherical_functions are LM_total_size, LM_range. Those two functions are pretty simple — do we really need to add spherical_functions as a dep?

From spherical_functions/__init__.py:

@njit('i8(i8,i8)')
def LM_total_size(ell_min, ell_max):
    """Total array size of (ell,m) components

    Assuming an array of

    [[ell,m] for ell in range(ell_min, ell_max+1)
             for m in range(-ell,ell+1)]

    this function returns the total size of that array.

    This can be calculated in sympy as

      from sympy import symbols, summation
      ell,ell_min,ell_max = symbols('ell,ell_min,ell_max', integer=True)
      summation(2*ell + 1, (ell, ell_min, ell_max))

    """
    return ell_max * (ell_max + 2) - ell_min ** 2 + 1

def LM_range(ell_min, ell_max):
    """Array of (ell,m) indices in standard order

    This function returns an array of essentially

    [[ell,m] for ell in range(ell_min, ell_max+1)
             for m in range(-ell,ell+1)]

    This is, for example, the order assumed for mode data in the `waveforms`
    module.

    """
    # # Sympy commands to calculate the total size:
    # from sympy import symbols, summation
    # ell_min,ell,ell_max = symbols('ell_min,ell,ell_max', integer=True)
    # summation((2*ell + 1), (ell, ell_min, ell_max))
    LM = np.empty((ell_max * (ell_max + 2) - ell_min ** 2 + 1, 2), dtype=int)
    _LM_range(ell_min, ell_max, LM)
    return LM


@njit('void(i8,i8,i8[:,:])')
def _LM_range(ell_min, ell_max, LM):
    i = 0
    for ell in range(ell_min, ell_max + 1):
        for m in range(-ell, ell + 1):
            LM[i, 0] = ell
            LM[i, 1] = m
            i += 1

@gda98re
Copy link
Author

gda98re commented Jan 24, 2025

No I think I can write them in the code and avoid using spherical_functions. Should I remove spherical_functions from the example notebook as well?

@duetosymmetry
Copy link
Collaborator

Only if you want to. We use other packages in example nbs — we just have to install them during testing so that test_example_notebooks.py will pass. For example:

conda install qnm sxs

@gda98re
Copy link
Author

gda98re commented Jan 24, 2025

Also, for code readability and consisetncy I was thinking about changing the fit keys from "sup_" -> "alpha_". Not sure if leaving the key "boost_velx" [y,z] for the boost velocity or something shorter, like "v_x"

@gda98re
Copy link
Author

gda98re commented Jan 24, 2025

ok, then maybe I can simply copy the short function to get the ell m mode index in the notebook and then add a note that the same thing is implemented as LM_index in spherical_functions package

@duetosymmetry
Copy link
Collaborator

duetosymmetry commented Jan 24, 2025

I like alpha! As long as the conventions are clear about which way the boost/velocity is going, I think both boost and v are easy to understand.

@gda98re
Copy link
Author

gda98re commented Jan 24, 2025

For the boost I could simplify as "boost_velx" -> "boost_x" also. The only reason I wanted to emphazises the word "boost" is to distinguish it from the kick velocity of other remnant models. It's true that the two concepts are substantially the same, but they are calculated in a very different way

@duetosymmetry
Copy link
Collaborator

I'm happy with boost — just have to be sure it's clear that it's the active boost to go from the PN BMS frame to the late-time superrest frame (or the opposite, if it happens to be the opposite)

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.

2 participants