Skip to content

Fix surface density, add cumulative surface density #161

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

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

Conversation

savchenkoyana
Copy link

No description provided.

@savchenkoyana
Copy link
Author

Dear maintainers,
thank you for this project! It is very useful!

I think there is a bug with surface density because of the order of operations: Pi * i? smth : anything <-- here multiplication executes before the ternary operator, so Pi gets lost.
I fixed this one and also introduced cumulative surface density

@teuben
Copy link
Owner

teuben commented Nov 30, 2024

thank you for the fix, we sadly don't have a full regression test, but I will check your PR this weekend and report here how it went.

@teuben
Copy link
Owner

teuben commented Apr 14, 2025

For the record, this is how I compared the results:

mkplum p1M 1000000
gyrfalcON p1M . tstop=0.001 eps=0.05 kmax=7 manipname=projprof manipfile=junk1M_new.txt manippars=1,0,0,100

the dot for the output file simplifies re-running it, as data is not written to a file.

@teuben
Copy link
Owner

teuben commented Apr 15, 2025

Indeed, a snippet of C code confirmed that the new expression is smaller by a factor of Pi.

@teuben
Copy link
Owner

teuben commented Apr 15, 2025

If the new third column is a cumulative surface brightness, should it not converge to the total mass? Right now it seems it is printing the contribution in a shell, i.e. Pi.M.r^2

@savchenkoyana
Copy link
Author

If the new third column is a cumulative surface brightness, should it not converge to the total mass? Right now it seems it is printing the contribution in a shell, i.e. Pi.M.r^2

PP.Mr(i) is M(<r), and PP.rad(i) is radius of bin:

https://github.com/teuben/nemo/blob/master/usr/dehnen/falcON/inc/public/profile.h#L119C1-L122C41

I made a small python script that compares Plummer density and parameters from projprof:

import numpy as np
import matplotlib.pyplot as plt

if __name__ == "__main__":
    data = np.loadtxt("junk1M_new.txt").T
    r_edges, sigma, sigma_cum = data[0], data[1], data[2]

    def plummer_sigma(r):
        return 1 / np.pi / (1 + r**2)**2

    def plummer_sigma_cum(r):
        return 1 / np.pi / (1 + r**2)

    # Compute bin centers from edges
    r_edges = np.hstack((np.zeros(1,), r_edges))
    r_centers = (r_edges[:-1] + r_edges[1:]) / 2.
    bin_widths = r_edges[1:] - r_edges[:-1]

    plt.bar(r_centers, sigma_cum, width=bin_widths, align="center", label="snapshot cum. surf. density", color="g")
    plt.plot(r_centers, plummer_sigma_cum(r_centers), label="theory cum. surf. density", color="y")
    plt.bar(r_centers, sigma, width=bin_widths, align="center", label="snapshot surf. density", color="r")
    plt.plot(r_centers, plummer_sigma(r_centers), label="theory surf. density", color="b")
    plt.yscale("log")
    plt.legend()

    plt.show()

{D09D2B06-CFD5-4CE3-8D1A-65D39FF2AC8A}

Did I miss something? Or was the question about something else (cumulative mass, not cumulative density)?

@savchenkoyana
Copy link
Author

I'm not very good with terms, what I wanted to add is M(<r) / (pi * r^2), where r is upper bound of a bin.

I don't know why I decided to put it in the same PR with bugfix. Maybe it is better to remove it for now? And then add it in another PR if it's useful

@teuben
Copy link
Owner

teuben commented Apr 16, 2025

Cumulative mass makes more sense, since it's something that converges to the total mass. But because of binning and where one places the "r", it probably is not mathematically the same. Ideally it should be.

Your plummer_sigma_cum() function above is indeed the cumulative mass function, although I think there doesn't need to be a PI in there. It converges to 1 for R->large

Cumulative density is not something I've thought about being useful for anything, can you elaborate?

@savchenkoyana
Copy link
Author

No, I think you're right, I removed it

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