- changed status to open
- removed comment
Add option to compute center of mass to hydro_analysis and lots of smaller changes
The attached set of patches adds the possibility to compute a center of mass location to hydro_analysis. It works by computing the center of mass (via x[idx]*rho[idx]) of the matter in a region of radius Hydro_Analysis_r_core around the point of maximum density. Only points whose density is above a fraction Hydro_Analysis_rho_core_rel_min of the maximum density are considered.
I also include a number of bugfix patches and patches to make Hydro_Analysis more controllable by user input rather than hard-coded values. All patches contain a short description in the patch (ie. the git commit message).
The ones adding extra control and/or warnings are: * Hydro_Analysis: add verbosity_level option to control how much data to output to stdout * Hydro_Analysis: use Fortran modules for prototypes if available * Hydro_Analysis: add parameters to control which interpolator is used * Hydro_Analysis: make parameters steerable * Hydro_Analysis: warn if grid point with maximum value cannot be found on grid * Hydro_Analysis: add parameter Hydro_Analysis_comp_rho_max_every * Hydro_Analysis: add option rho_max_loc_use_rotatingsymmetry180
- Hydro_Analysis: add option to average the location of multiple identical maxima
- Hydro_Analysis: fiddle with when to warn about multiple identical maxima
Bugfixes are: * Hydro_Analysis: checkpoint grid scalars * Hydro_Analysis: do reduction in global mode
New feature: * Hydro_Analysis: add routine to compute center of mass in core region
Keyword: Hydro_Analysis
Comments (19)
-
reporter -
reporter - removed comment
Anyone? Please review the bugfixes?
-
- removed comment
0001-Hydro_Analysis-add-option-to-average-the-location-of.patch looks good so far, but I believe it would be nice to be able to say 'allow this only to happen if the identical values are actually within up to 2*dx from each other. That however would require to actually store these locations. Please apply 0001.
-
- removed comment
0002-Hydro_Analysis-add-verbosity_level-option-to-control.patch looks good too.
-
- removed comment
0003-Hydro_Analysis-use-Fortran-modules-for-prototypes-if.patch: I assume this is preparing following patches, e.g., 0004. Now, these then use the table-interfaces from Fortan. Wouldn't that make this not only optional but required? Do later patches compile without the FORTAN capability?
-
- removed comment
0007: please apply (once these parameters are all introduced)
-
reporter - removed comment
Replying to [comment:5 knarf]:
0003-Hydro_Analysis-use-Fortran-modules-for-prototypes-if.patch: I assume this is preparing following patches, e.g., 0004. Now, these then use the table-interfaces from Fortan. Wouldn't that make this not only optional but required? Do later patches compile without the FORTAN capability?
Fortran does not require prototypes and all functions are all usable without the module. So this patch is purely optional. I like having prototypes if possible since they help me catch errors.
All patches seem to compile fine without Fortran modules present. I just verified this by removing Fortran from my thornlist, which also disables QuasiLocalMeasures.
-
reporter - removed comment
Applied 0001 and 0002 as rev 122 and 123 of Hydro_Analysis.
-
- removed comment
Please commmit 0003 then. I didn't have time for the rest yet, sorry.
-
reporter - removed comment
Committed 0003, 0004 and 0005 as rev 124, 125 and 126 of Hydro_Analysis (0004 and 0005 modify only warnings are default to the old behaviour). I fully expect this to take a while since some patches are quite extensive. Thank you for taking the time.
-
reporter - removed comment
Updated 0006 to replace an assert() by a VWarn(1,...) and early return (the situation assert'ed against actually happens regularly in NSNS mergers).
-
reporter - removed comment
ping.
-
- removed comment
Which of these patches still need to be applied (and can they)?
-
reporter - removed comment
0006, 0007, 0008, 0009, 0010, 0011, 0012, 0013, 0014 all should be applied. They all apply on top of the current trunk. 6, 9 are updated since they contained an update (6) or no longer applied (9). 10 was missing from the first batch, 13 and 14 are new.
-
- removed comment
The centre of mass is not
rho x^i / M
, but rathersqrt(det(g_ij)) rho x^i / M
, whereM = sqrt(det(g_ij)) rho
(integral signs omitted).I don't see how myrho is defined in patch 0006, and it's not currently defined in Hydro_Analysis either. Maybe myrho = rho sqrt(g)?
-
reporter - removed comment
myrho is declared by DECLARE_MY_VARIABLE(rho) in line 44. I am fine renaming the routine to "centroid" rather than "center of mass". Since this is only used to track the star and move the grids along using just rho is fine. For the actual center of mass one would either have to compute sqrt(det(g_ij)) * w_lorentz * rho or integrate dens. This would be more expensive. It is very easy to change the code so that it takes a parameter specifying what "rho" should be "HYDROBASE::rho" would be the current situation and "GRHYDRO::dens" would be the actual center of mass. Would that be acceptable?
-
- removed comment
It's okay to only implement what you need. If you don't need the true CoM, then there's no need to calculate it. Renaming it to "centroid" (and adding a comment that that is not actually the centre of mass) is fine.
-
- changed status to open
- removed comment
-
reporter - changed status to resolved
- removed comment
Applied as git hashes
0600cfc (0006) f68e878 - rename CoM to centroid 3474885 (0007) e6ca32e (0008) 2b3613b (0009) 88c0f56 (0010) 4d7d0e8 (0011) 9df58fc (0012) 320dd23 (0013) 3b4310e (0014) 448bdad - adds a test case e97ae9e - use "-" to separate thorn and variable name
- Log in to comment