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

Improvements to the Radial Range that Subhalo Density Profiles are Tabulated when Fitting TNFW Profile #483

Merged

Conversation

cgannonucm
Copy link
Contributor

Changed the range of radii at which the density profile is tabulated at when fitting TNFW profiles. Before the density was tabulated to the outer radius (where the radius encloses the bound mass or the total mass (of the subhalo), whichever is smaller). This definition led to the maximum radius taking on very large values in some cases. To fix this, the following changes have been implemented:

  • Now the maximum tabulated radius is the virial radius if the outer radius is less than the virial radius
  • If the outer radius is greater than the virial radius, the maximum radius is the radius at which the density drops to 1/10 the density at the virial radius or 10x the virial radius, whichever is smaller

Also fix some minor formatting and spelling issues.
if (radiusOuter > radiusVirial) then
radiusMaximumCountRadii=int(log10(radiusMaximumScaleVirialMaximum)*dble(radiusMaximumCountRadiiPerDecade)+1.0d0)
factorStepRadius = log10(radiusMaximumScaleVirialMaximum)/dble(radiusMaximumCountRadii )
do i=1,radiusMaximumCountRadii
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was previously:

         do n=1,radiusMaxiumCountRadii
             radiusMaximum                     =10**(log10(radiusVirial)+step*dble(i))

So the loop variable was n but on the following line it used i to increment radiusMaximum. Most likely i=0 initially, so this would not have actually increased radiusMaximum, leaving it always equal to radiusVirial. I've corrected this (just using i as the loop variable), but it would be good if you can check if this still gives good results for the fits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I recompiled and re-ran the parameter with the bug fixed. The fits look good.

Copy link
Collaborator

@abensonca abensonca left a comment

Choose a reason for hiding this comment

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

Overall this looks good - I saw one issue with a difference between the name of a loop variable and the variable actually used in the loop (see other comment). I've fixed this, but let's check if the results still look good after this change.

Copy link
Collaborator

@abensonca abensonca left a comment

Choose a reason for hiding this comment

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

All tests have already passed, and since the fits still look good with the previous fix, this is ready to merge.

@abensonca abensonca added this pull request to the merge queue Sep 28, 2023
Merged via the queue into galacticusorg:master with commit 563a052 Sep 28, 2023
260 checks passed
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