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

Mistakes in metallity calculations #123

Open
MatthieuSchaller opened this issue Feb 22, 2022 · 1 comment
Open

Mistakes in metallity calculations #123

MatthieuSchaller opened this issue Feb 22, 2022 · 1 comment

Comments

@MatthieuSchaller
Copy link

MatthieuSchaller commented Feb 22, 2022

We identified two issues when computing the metallicities of substructures.


First one is a missing normalisation of the aperture-based total-gas Z.

This patch should fix it:

diff --git a/src/substructureproperties.cxx b/src/substructureproperties.cxx
index f5da189..3fd8a0f 100644
--- a/src/substructureproperties.cxx
+++ b/src/substructureproperties.cxx
@@ -5580,7 +5580,7 @@ void CalculateApertureQuantities(Options &opt, Int_t &ning, Particle *Part, Prop
             if (EncMassGas>0) pdata.aperture_vrdisp_gas[iaptindex]=EncVRDispGas/EncMassGas;
 #ifdef STARON
             pdata.aperture_SFR_gas[iaptindex]=EncSFR;
-            pdata.aperture_Z_gas[iaptindex]=EncZmetGas;
+            pdata.aperture_Z_gas[iaptindex]=EncZmetGas/EncMassGas;
             pdata.aperture_npart_gas_sf[iaptindex]=NinsideGasSF;
             pdata.aperture_npart_gas_nsf[iaptindex]=NinsideGasNSF;
             pdata.aperture_mass_gas_sf[iaptindex]=EncMassGasSF;

Basically, the value returned is \sum m_i Z_i with the division by \sum m_i at the end missing.
This is done correctly for the SFing and NSFing sub-components, however.

I haven't checked whether there are other similar problems but all the other outputs look correct when looking at our pipeline plots.


Second one is related to OpenMP and non-aperture quantities.
It seems that at least Zmet_gas and Mass_gas are not calculated correctly when running with OpenMP.

This plot shows the issue:
masses
We'd expect Mass_gas to be Mass_gas_sf + Mass_gas_nsf.
This problem then translates into incorrect Z for the total.

If I switch off openMP then the problem is not there any more.

I think this means there is a problem with the private variables used for this calculation.
There might also be additional problems with other quantities; we haven't checked everything.

@MatthieuSchaller
Copy link
Author

The projected aperture metallicites also seem to be weird. But I couldn't come up with a sensible patch; I don't understand well enough how this whole code section works.

rtobar added a commit that referenced this issue Apr 14, 2022
This was pointed out in #123, and while a patch was provided I followed
the style/checks from the surrounding code that avoid divisions by zero.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
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

No branches or pull requests

1 participant