Uninitialised gas SFR threshold #79
Comments
|
Interesting. Might be related to / leading to #75. Now, unfortunately, I don't know what that variable ought to be doing... Definitely needs to be initialised. Whether it needs to be a parameter I don't know. For EAGLE-related applications that will always be zero anyway. |
|
OK, I'll add a default value of |
This variable was not being initialised, and is never written to, and thus code reading it cannot assume any consistent value, even between two different runs. This was reported in #79. Additionally we might want to add a new option in the configuration file to allow users provide an arbitrary value for this setting. Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
|
The change is now on the |
|
Hi @rtobar @MatthieuSchaller it is the variable that is used to split star forming gas from non-star forming gas so 0 is a good default value. I've just pushed a branch with a fix (default is 0 and has configuration option). Did you want to try |
|
Thanks @pelahi for the very prompt reply! I'll cherry-pick the main bit from pelahi@875a60d to allow users to provide the setting. |
|
The option is now defaulted to |
|
Thanks! Just to be extra clear, do we now need to update our config files? |
|
@MatthieuSchaller no, it shouldn't be necessary as the default value of |
The Options class' members were all first declared, then initialised in the body of the default constructor. This is firstly against the idea of initialising members in the member initialisation list, but moreover it required having to keep two lists of members in the code: the one declaring them, and the one initialising them. This commit brings together member declaration and initialisation, which has been available since C++11. While doing so I found a few members that were: * Not initialised (see #79 for example). * Initialised later on deep within VR (so probably shouldn't belong in this class in the first place). * Uninitialised and later on only written to (and never read). * Not used at all. I've removed the latter two sets of members, and added initialisation for the first two. I've also taken the opportunity to remove all the rest of the member functions, including the constructor, as they were unnecessary. Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
While reading the code I found that
Option.gas_sft_thresholdis never initialised: it doesn't have a default value (so it cannot be assumed it takes a fixed value at startup), and there's no place in the code that writes into it (either from the command line, configuration file, or input data files). On the other hand the value is used in a few places in the code, particularly in few of the functions insubstructureproperties.cxx.Given the variable is uninitialised, this potentially means some results (basically whatever happens within those
ifs) cannot be guaranteed to be consistent across different compilations/settings/platforms/etc.The fix should be easy: firstly, make sure the variable has a default value, and secondly, we'll probably need a way to assign arbitrary values for this threshold.
@MatthieuSchaller, I have two questions about this:
0is a good default value, so I'll go with that. For setting arbitrary values: do you reckon they are supposed to be given via the configuration file (via a new setting), or should they somehow be computed from the input data?The text was updated successfully, but these errors were encountered: