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

Fix some minor problems in boost calculation and improve docstring #646

Merged
merged 11 commits into from
Oct 18, 2024

Conversation

combet
Copy link
Collaborator

@combet combet commented Oct 3, 2024

  • prevent rscale from being an optional keyword with default value (1000) as it must have the same units as rvals. I think it's safer for the user to have to provide a value.
  • make corresponding changes
    • in utils.py
    • in boost notebooks
    • in test_utils
  • Add equations to the docstrings to have a nice rendering in the documentation
  • Rename functions correct_sigma_with_boost* as the boost will generally be applied to DeltaSigma or shear and not Sigma.

@combet combet self-assigned this Oct 3, 2024
@combet combet marked this pull request as draft October 3, 2024 11:03
@coveralls
Copy link

coveralls commented Oct 3, 2024

Coverage Status

coverage: 100.0%. remained the same
when pulling a3d4ae0 on issue/645/improve_boost
into 157bdb0 on main.

@combet combet marked this pull request as ready for review October 3, 2024 13:16
Copy link
Collaborator

@hsinfan1996 hsinfan1996 left a comment

Choose a reason for hiding this comment

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

Thank you.
In the notebook demo_boost_factors.ipynb, is it expected to get nan from print(u.compute_nfw_boost(1000, 1000, boost0=0.1)) (cell 10)?

clmm/utils/boost.py Outdated Show resolved Hide resolved
clmm/utils/boost.py Outdated Show resolved Hide resolved
@combet
Copy link
Collaborator Author

combet commented Oct 3, 2024

Thank you. In the notebook demo_boost_factors.ipynb, is it expected to get nan from print(u.compute_nfw_boost(1000, 1000, boost0=0.1)) (cell 10)?

Ah right, I forgot to remove that line in the notebook... I was checking things out. I think that's not really a problem as the denominator will go to zero when x=1, so that's expected. Question is whether we want to add a warning or something to explicitely deal with the case R_proj = R_s?

@hsinfan1996
Copy link
Collaborator

hsinfan1996 commented Oct 3, 2024

For completeness, yes, we should probably add something to deal with the case R_proj = R_s. My guess is that the value F(x) at x = R_proj / R_s = 1 is just a constant, so we can fill the value in the output array and don't need to show warnings.

clmm/utils/boost.py Outdated Show resolved Hide resolved
@hsinfan1996
Copy link
Collaborator

We can do the following to deal with the case R_proj = R_s:

    r_norm = np.array(rvals) / rscale

    def _calc_finternal(r_norm):
        radicand = r_norm**2 - 1

        finternal = (
            -1j
            * np.log(
                (1 + np.lib.scimath.sqrt(radicand) * 1j) / (1 - np.lib.scimath.sqrt(radicand) * 1j)
            )
            / (2 * np.lib.scimath.sqrt(radicand))
        )

        return np.nan_to_num((1 - finternal)/radicand, copy=False, nan=1.0/3.0).real

    return 1.0 + boost0 * _calc_finternal(r_norm)

@combet
Copy link
Collaborator Author

combet commented Oct 10, 2024

We can do the following to deal with the case R_proj = R_s:

    r_norm = np.array(rvals) / rscale

    def _calc_finternal(r_norm):
        radicand = r_norm**2 - 1

        finternal = (
            -1j
            * np.log(
                (1 + np.lib.scimath.sqrt(radicand) * 1j) / (1 - np.lib.scimath.sqrt(radicand) * 1j)
            )
            / (2 * np.lib.scimath.sqrt(radicand))
        )

        return np.nan_to_num((1 - finternal)/radicand, copy=False, nan=1.0/3.0).real

    return 1.0 + boost0 * _calc_finternal(r_norm)

Thanks, I implemented that and that took care of the issue!

@combet
Copy link
Collaborator Author

combet commented Oct 10, 2024

For completeness, yes, we should probably add something to deal with the case R_proj = R_s. My guess is that the value F(x) at x = R_proj / R_s = 1 is just a constant, so we can fill the value in the output array and don't need to show warnings.

Done. And docstrings updated accordingly.

@hsinfan1996
Copy link
Collaborator

hsinfan1996 commented Oct 10, 2024

@combet Thank you. I will approve the PR.

Copy link
Collaborator

@m-aguena m-aguena left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks @hsinfan1996 for doing the hard part of the review!

@m-aguena m-aguena merged commit e7164d2 into main Oct 18, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

boost.py - Remove default value for r_s + fix docstring + improve function names
4 participants