Multiple metallicity variables

[LewisPicker]

Describe the bug In BaseStar.h there is multiple definitions of metallicity (Rho, Sigma and Xi) are defined to be used with the Hurley models. IM noticed this and thought it could cause some problems if changes to these variables are not reciprocated.

Label the issue

severity_minor - This is a minor bug with minimal impact

[jeffriley]

@LewisPicker @ilyamandel can you elaborate... What changes are you concerned about, and reciprocated where?

Fyi, the values of these variables are all set in the BaseStar constructor:

m_LogMetallicityXi    = log10(m_Metallicity / ZSOL);
m_LogMetallicitySigma = log10(m_Metallicity);
m_LogMetallicityRho   = m_LogMetallicityXi + 1.0;

So they are all based on the value of m_Metallicity and ZSOL - does that alleviate the concern?

[ilyamandel]

@jeffriley -- my usual concern with keeping multiple copies of the same underlying information is that someone could change one but not the other, leading to subtle bugs. In this case, for example, I would prefer to have getter functions for these additional variables that would simply return log10(m_Metallicity / ZSOL), etc.

[jeffriley]

@ilyamandel I don't think they are ever used outside the BaseStar class are they? They are calculated once only inside the constructor, and they are really only there so they don't have to be recalculated every time they are used (at least once every timestep). Getter functions would introduce overhead that isn't necessary - unless you call the getters once from the constructors and put the results into the class variables I guess, but then you're exposing the getters for people to use instead of the class variables and inadvertently causing extra overhead. Putting the calculations in getter functions won't prevent someone from changing one but not the other - maybe it would make it a more deliberate change and help prevent an inadvertent change - maybe - but I don't think that outweighs the possible extra overhead. But my opinion is just one opinion...

[ilyamandel]

I meant that I'd rather recalculate them (and take the computational cost, which isn't large -- they aren't used that often) than risk them being corrupted. The fact that they aren't currently being used or set outside of BaseStar doesn't mean they couldn't be -- someone might decide to change one but not another of these in one of the many classes that inherit from BaseStar, for example.

[jeffriley]

I'm not convinced, but don't really have a strong preference either way.

[jeffriley]

I'd really like to not have to call log10() too many times though - it's not cheap... How about a compromise - keep log10(m_Metallicity) and log10(ZSOL) as class variables, calculated in the BaseStar constrcutor, then have getters that use those to calculate the values of m_LogMetallicityXi, m_LogMetallicitySigma, and m_LogMetallicityRho?

Edit: log10(ZSOL) could (probably should) be calculated as a constexpr in constants.h

[jeffriley]

@ilyamandel How's this: PR #891 ?

[ilyamandel]

Looks good, happy to close this!