Closed wdconinc closed 11 months ago
@nathanwbrei I looked at this and it looks like the issue is with the template specializations at the bottom of JParameterManager.h. If lhs == 0
then the <
comparison will fail since the comparison is being made between 0
and 0
.
This is an easy fix by just making it <=
. My question though is whether this should include a check on the (scaled) std::numeric_limits<double>::epsilon()
value at all. (maybe this is what you were referring to the other day in our discussion). Was there a situation that motivated using epsilon instead of a straight ==
comparison?
As a reminder, my main motivation for having this mechanism in the first place is to ensure that any compiled number used in a job (e.g. a default value), can be dumped as part of the full configuration at the end of the job and then read it on a subsequent job and have some guarantee of reproducibility. That may only be possible with a strict equality.
ensure that any compiled number used in a job (e.g. a default value), can be dumped as part of the full configuration at the end of the job and then read it on a subsequent job and have some guarantee of reproducibility
So, if I understand that correctly, if we are only using values hard-coded in compiled C++ code (which is exclusively what we are doing), then any time such a message appears it is by definition a reportable issue? This also affects the non-zero values, and it's not attributable to different types and epsilons, I think:
float m_cotThetaMax
is defined here, https://github.com/eic/EICrecon/blob/main/src/algorithms/tracking/OrthogonalTrackSeedingConfig.h#L33.float cotThetaMax
here, https://github.com/eic/EICrecon/blob/main/src/algorithms/tracking/OrthogonalTrackSeedingConfig.h#L138, where that float
is defined here, https://github.com/acts-project/acts/blob/v21.1.1/Core/include/Acts/Seeding/SeedFinderOrthogonalConfig.hpp#L31SetDefaultParameter
here, https://github.com/eic/EICrecon/blob/main/src/global/tracking/TrackSeeding_factory.cc#L49And it produces the warning:
[WARN] Parameter 'tracking:CentralTrackSeedingResults:cotThetaMax' with value '27.2899' loses equality with itself after stringification
Yes, the warning is valid in this case because you are using a compiled value rather than an externally supplied value AND the value is not perfectly representable using the default precision in std::stringstream
. The first line you referenced is
float m_cotThetaMax = 1.0 / tan(2. * atan(exp(-4.0)));
Here, the author has chosen to make the calculated value the configuration parameter rather than say, the -4
or even something like: ThetaMax_degrees=2.1
. Both of those might be better choices for the actual config. parameter simply because they are easier for users to read and interpret in external formats. (Most mortals would need to use a calculator to know what angle cot(theta)=27.2899 corresponds to.)
In this specific example std::stringstream
converts the number to the string 27.2899
using the default precision. The actual number in the variable is 27.2899169921875
. If a user ran a job and dumped the config. parameters to a file (--dumpconfigs
) and then tried to reproduce the results later by specifying that file as the configuration at run time, they may not see identical results. Thus, the warning is being correctly given.
There are a few ways to resolve this. One would be to change JANA to set the precision for std::stringstream
conversions to something much higher than the default. I think this might be a good thing to do, but would like to hear if there are other opinions.
Another is allow the user to specify that they don't want such warnings to be printed.
A third, of course, is for the user to adopt a convention for configuration parameters that favors easy representation in external formats (configuration DBs or run-time input files).
I really think the content of the config file should match the user's intent, not the machine's internal representation.
What if we added a precision
field to JParameter
, which we would use to stringify floating point values back to whatever the user intended? If the user provides a precision that doesn't match the value they specified, they get this warning.
Yet another option is to avoid these Stringify(Parse())
round trips completely by specifying the parameter default value as a string to begin with. We can do this already using SetParameter()
, although I'd advocate adding T JParameterManager::RegisterParameter(std::string name, const std::string default_val, std::string description)
(The current RegisterParameter
has a typed default, which provides more compile-time safety at the expense of headaches like this one)
Here is our new approach, which I believe addresses everyone's concerns:
JParameterManager::Equals()
now checks strict equality instead of epsilon equality
Floating point values are serialized using precision std::numeric_limits<T>::max_digits10
, which is defined to be the minimum precision needed to guarantee an exact T -> string -> T
round trip. (Fun fact: using a higher precision results in junk data appended to the stringified version.)
std::stringstream
is smart enough to detect when a floating point value has an exact decimal representation and stop printing zeros, so 0.0 is stringified to "0" and not "0.00000000000000000" without us having to do anything tricky. Cleanly representable values stay clean, whereas real numbers become long and ugly, but stay accurate.
With JANA2-2.1.0, we get plenty of warnings "Parameter X with value 0 loses equality with itself after stringification" for parameters where the conversion should really not be a problem (not affected by floating point representation). Here is a set of cases where the warning appears but likely can be avoided. This refers to the EICrecon code base.
Example 1
The config struct with
depthCorrection
which defaults to zero, which is set to zero in the factory instantiation, and triggers thisSetDefaultParameter
lineExample 2
The class member
m_tRes
which defaults to zero, is set to zero in the factory, and then passed to thisSetDefaultParameter
lineImpact
In total this ends up looking like this: