Re: [lmi] Ancient rate-table programs: GIGO

[Vadim Zeitlin]

On Sat, 10 Dec 2016 22:16:55 +0000 Greg Chicares <address@hidden> wrote:

GC> Kim has discovered a new class of errors in historical rate tables.
GC> A text-format table that looked like this in relevant part [note the
GC> inconsistency]:
GC> 
GC> Number of decimal places: 6
GC> Table values:
GC> ...
GC>  25  0.00002551
GC> 
GC> [spoiler alert--the inconsistency is identified below]
GC> 
GC> was merged into a binary {.dat .ndx} blob

 Sorry, I'm afraid I've lost the big picture here. Do I understand
correctly that the goal is to accept such binary tables on input and
produce output with the correct number of decimal places (8) for them?

GC> Here is a patch that I will probably commit on the belief that it
GC> does no harm yet adds some value:
GC> 
GC> 
--------8<--------8<--------8<--------8<--------8<--------8<--------8<--------
GC> diff --git a/rate_table_tool.cpp b/rate_table_tool.cpp
GC> index c0cb113..0fe90b1 100644
GC> --- a/rate_table_tool.cpp
GC> +++ b/rate_table_tool.cpp
GC> @@ -340,7 +340,17 @@ int verify(fs::path const& database_filename)
GC>                      << LMI_FLUSH
GC>                      ;
GC>                  }
GC> -
GC> +            if(new_table != orig_table)
GC> +                {
GC> +                // This is not really fatal, it is only used here to throw 
an
GC> +                // exception in a convenient way.
GC> +                fatal_error()
GC> +                    << "After loading and saving the original table #"
GC> +                    << num
GC> +                    << "binary contents differed.\n"
GC> +                    << LMI_FLUSH
GC> +                    ;
GC> +                }
GC>              }
GC>          catch(std::exception const& e)
GC>              {
GC> @@ -395,6 +405,10 @@ int verify(fs::path const& database_filename)
GC>              {
GC>              table const& orig_table = orig_db.get_nth_table(i);
GC>              table const& new_table = new_db.get_nth_table(i);
GC> +            if(new_table.compute_hash_value() != 
orig_table.compute_hash_value())
GC> +                {
GC> +                std::cout << "CRC mismatch: " << orig_table.number() << 
std::endl;
GC> +                }
GC>              if(new_table != orig_table)
GC>                  {
GC>                  std::cout
GC> 
-------->8-------->8-------->8-------->8-------->8-------->8-------->8--------
GC> 
GC> The second chunk might indeed do nothing. My guess is that a similar
GC> or identical condition is tested upstream, throwing an exception like
GC> 
GC>   Verification failed for table #140: Error reading table from string:
GC>   bad data for table 140: hash value 2893035046 doesn't match the
GC>   computed hash value 1855339846.

 Yes, this check is redundant.

GC> on failure. However, the first chunk definitely finds problems that
GC> aren't otherwise found. Without that patch chunk, verify() performs
GC> this chain of conversions:
GC> 
GC>   bin0 --> txt1 --> bin2 --> txt3
GC> 
GC> and compares txt1 and txt2
                             ^
 (I assume this was just a typo and "txt3" was meant)

GC> (this change compares bin0 and bin2 as well: an independent condition).

 I wouldn't say "independent", but it is indeed not identically the same
currently because I hadn't realized we could lose information during "bin
-> txt" conversion if the binary table contained numbers in a greater
precision than its "number of decimals" field indicated.

 The only way to ensure that this change is not needed would be to make
certain that we never lose (or even change) any data when doing this
conversion, however this would mean refusing to load binary files with the
wrong "number of decimals" specified in them but, if my guess in the
beginning of this email is correct, this isn't the right thing to do, so we
can't really achieve this.


GC> Here is a separate patch that isn't suitable for production in its
GC> present form, but I'm using it for experimental investigations.

... [patch and its explanation snipped] ...

 I think it would make sense to integrate this patch into production,
possibly after improving it, as this would seem to be the right (or the
"least wrong", if you prefer) thing to do in this case. Surely doing what
we do now, i.e. silently losing the data if the number of decimals is
wrong, is much worse than that.

 However notice that if such patch were integrated into production code,
then the extra check added by the first chunk of the first patch above
risks to be harmful because it would still be triggered (due to the
different values of the "number of decimals" field in "bin0" and "bin2"),
even though everything would work [as] well [as it could possibly can].

GC> It issues many false positive warnings such as:
GC>   Table specifies 8 decimals but 19 are required.
GC> where the last ten digits are '9999999997' or '0000000002'; clearly
GC> it would be more useful if I refined that. Such warnings cannot be
GC> dismissed out of hand: table #150 in the same proprietary database
GC> has values with seventeen significant digits at ages 83-85, but at
GC> other ages values actually were rounded to the specified number of
GC> decimals, so this is probably a spreadsheet error.

 Should I do anything about this, e.g. work on refining this patch or help
with it in any other way?

GC> Other warnings like
GC>   Table specifies 8 decimals but 6 are required.
GC> indicate tables that explicitly include insignificant trailing zeros
GC> that we should remove.

 But we could also leave them unchanged, there is no real harm in this, is
there?


 Sorry for missing this problem, it's really aggravating that even after
doing my best to ensure that we didn't lose data when round tripping
between binary and text formats, I still managed to miss a case in which
this does happen. And thanks to Kim for finding it, of course!

 Once again, please let me know if I can do anything to help with fixing it
now, thanks in advance,
VZ