Re: [lmi] Code review request

[Greg Chicares]

On 2020-09-22 14:09, Vadim Zeitlin wrote:
> On Tue, 22 Sep 2020 11:26:35 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
> 
> GC> Vadim--We have this pattern, for data used in reports:
> GC>  - data vary by context (e.g., by input fields such as 'ProductName')
> GC>  - store all context-specific values in some data structure somewhere
> GC>  - look them up in context, to generate reports
> GC> Often we store all potential values in a std::map--e.g.,
> GC>  - title_map_t static_titles() in 'ledger_evaluator.cpp'
> GC> and look them up by enumerators--e.g.,
> GC>  - finra_assumption_detail's anonymous local enum in 'pdf_command_wx.cpp'
> GC> just to cite a couple of examples in code you've worked on. Elsewhere:
> GC>  - 'dbnames.hpp' specifies an enum for the lookup key
> GC>  - 'dbdict.cpp' specifies product-specific values for each key, e.g.,
> GC>    in sample::sample(), and they're stored in product-specific XML files
> GC>  - 'ihs_basicval.cpp' reads those files and selects values in context, 
> e.g.:
> GC>      database().query_into(DB_MaturityAge   , EndtAge);
> GC> Now we have a similar need, and before making any final decision about
> GC> a design to meet it, I'd like to ask whether this "map-lookup-by-enum"
> GC> style seems appropriate...or is there some better alternative?
> 
>  I'm sorry, but I'm afraid I don't understand the question as it's not
> clear to me what exactly is part of the requirements and what is part of
> the current implementation. Literal reading of the above would seem to
> imply that we need to be able to look up values by an enum, in which case
> the question becomes rather tautological: there can be no better
> alternative to looking up by enum if this is what we _have_ to do. But I
> strongly suspect I'm just missing the point completely.

The paragraph preceding (up to "Now we have...") describes the current
implementation. It means: "Here's what we've already done historically
in similar circumstances". And the question is: "Now that we're about
to address a new set of similar circumstances, should we choose some
other way?"

But in light of your response, let me pose a somewhat different question:
Is the historical practice so atrociously bad that it would be wrong
to apply it in new circumstances? And is there some obvious alternative
that is vastly better? I don't really care if the choices I've made in
the past now seem arguable, or that some plausible alternative exists.

For example, if I were to suggest writing C++ code today that manages
memory in the old C way, with malloc()...free() pairs, or even in the
1980s C++ way, with new()...delete(), in some general application code
rather than a highly specialized library, then you might intervene,
declaring that friend don't let friends do that in this day and age.
It'd be the same if I proposed ignoring RAII in favor of garbage
collection because some other language does that.

I used to use a wordstar-like editor, and CVS or SVN. You insisted
that vim and git really are better. You were right. If the present
topic is like that, then I should listen.

OTOH, you advocate vim-fugitive, while I say I'm happy typing git
commands. My reluctance in this regard may seem a pity, but it does
not constitute presumptive malpractice.

So...is my database-string proposal at worst a pity? Or is it so
woeful that, if you were a member of a universal parliament, you
would pass a law making it a crime? Put that way, the question does
seem to answer itself. But maybe this is a gray area, and you're
itching to show me another way that I might recognize as clearly
superior?

>  To help me understand it, could you please explain what exactly do we need
> to be able to do? Also, there are probably parts of the existing
> implementation that are completely impractical to change (e.g. XML might
> not be the perfect format, but it could be out of question to consider
> changing it because of the numerous existing data files), but I'm not
> exactly sure what they are, i.e. what could be conceivably changed and what
> can't be touched at all?

XML is like, say, landline telephony. It's not the latest, most glamorous
way, but it's robust and mature--there aren't many "unknown unknowns".
Sometime in the future we might want to step back and choose something
different (e.g., you suggest SQLite below), but I'd much rather not think
about making such a change now, because XML is quite adequate and we have
lots of well-debugged XML code.

What do we need to be able to do? Make strings vary across some axes.
US state is the axis that motivates us most strongly. Given that we already
have a fixed set of seven '.database' axes that meet our every '.database'
need, I don't want to contemplate new axes. But '.product' files cannot
vary by any axis...yet they must. We've kludged around that in disparate
ways, all of which seem shameful now. Maintaining those is nightmarish;
extending them is unthinkable. We need strings to vary across some axes,
and we have a really good set of axes elsewhere, so we should just let
strings vary across those axes.

> GC> This "similar need" is exemplified in branch odd/string_db, which I've
> GC> pushed to savannah. It's a single-commit throwaway branch for discussion
> GC> only: an actual working implementation for a single lookup, described
> GC> in 'ledger_invariant_init.cpp'.
> GC> 
> GC> We already have '.database' files that hold arithmetic scalars that can
> GC> vary by up to seven parameters ("axes") for each product, and '.policy'
> GC> files that hold strings--which today vary by product only, but need to
> GC> vary by the same parameters '.database' files allow.
> 
>  This is probably a very naive question, but why can't we just put the
> .policy files contents into .database files? Is it just because changing
> the code dealing with the .database files to accept strings, i.e. use
> something like "std::variant<bool, int, double, std::string>" instead of
> just "double" in database_entity, so complicated that we can't
> realistically afford to make this change?

Yes. It took a lot of thought to design database_entity::operator[]()
and similar code, which IIRC is tightly bound to the idea behind its
  std::vector<double> data_values_;
member--that 'double' is the universal datatype that can hold anything
except a string. I shudder to think of replacing it with std::variant.
I know I can assign
  data_values_[some_index] = (bool) false;
or 3.14f or 720, and it'll all just work. But
  data_values_[some_index] = "Some string.";
just scares me. It don't imagine a trivial one-line change would just work:
-  std::vector<double> data_values_;
+  std::vector<std::variant<whatever>> data_values_;
How much more existing code would have to be changed? What could go wrong?
What unknown number of "unknown unknowns" lurk here? Hence my trepidation.

I haven't lost the ability to introspect, so I recognize that my reaction
is emotional and not logical. If I'm missing something wonderful in this
discussion, it's probably exactly this.

And indeed one of my early ideas was just to do:
  char* const policy_form_for_sample_product_generic {"UL32768"};
  char* const policy_form_for_sample_product_idaho {"UL32768-ID"};
  char* const policy_form_for_sample_product_texas {"UL-TX-1234-a"};
  ...
and then populate '.database' with addresses:
  int dims[e_number_of_axes] = {1, 1, 1, 1, 1, e_max_dim_state, 1};
  std::vector<int> dims(ptd, ptd + e_number_of_axes);
  std::vector<double> policy_form(e_max_dim_state, 
(double)&policy_form_for_sample_product_generic);
  policy_form[mce_s_ID] = (double)&policy_form_for_sample_product_idaho;
  policy_form[mce_s_TX] = (double)&policy_form_for_sample_product_texas;
  Add({DB_PolicyForm, dims, policy_form});
so that we could retrieve 'PolicyForm' in context thus:
  char const* const policy_form = b->database().query<char 
const*>(DB_PolicyForm);
Isn't that pretty much the same thing?

> GC> In several cases we've used expedients like this:
> GC> 
> GC>     auto const smoke_or_tobacco = 
> b->database().query<oenum_smoking_or_tobacco>(DB_SmokeOrTobacco);
> GC>     if(oe_tobacco_nontobacco == smoke_or_tobacco)
> GC>         {
> GC>         switch(b->yare_input_.Smoking)
> GC>             {
> GC>             case mce_smoker:    Smoker =    "Tobacco"; break;
> GC>             case mce_nonsmoker: Smoker = "Nontobacco"; break;
> GC>             case mce_unismoke:  Smoker = "Unitobacco"; break;
> GC>             }
> GC>         }
> GC>     else if(oe_smoker_nonsmoker == smoke_or_tobacco)
> GC>     ...
> GC> 
> GC> where a string must vary by a '.database' axis, but '.database' files
> GC> cannot hold strings.
> 
>  Right now they can't, but couldn't this be changed?

I was assuming that couldn't be changed. But maybe it can, with std::variant
(above), which I'm trying to understand, but the medication for my tooth
extraction is making that really hard.

> Also, in this
> particular case it doesn't seem too bad to do the translation in the code
> like this because the strings are elements of a small fixed set, i.e. the
> actual value is an enum, not a string.

What does "the actual value" mean? There are two cooperating types here:
 - small fixed sets of short strings, from which one is chosen by
 - an enumerative selector,
so I'd say their combination is actually a compound of those two types.
The selector resides in a '.database' file, and the switch statement
assigns or returns a std::string that might otherwise reside in a
'.policy' file (because it's a string).

Tomorrow, a new product could require a new set of strings. The
existing set encompasses different meanings:
 - marijuana smokers are smokers, but not tobacco users, while
 - tobacco chewers are tobacco users, but nonsmokers
and one could imagine a nicotine vs. non-nicotine distinction that
would include synthetic nicotine not derived from tobacco. Having
to change a switch statement for stuff like that is what I'd rather
avoid; we could do that by letting every '.policy' file specify
  TermForSmoker
  TermForNonsmoker
because in practice people are always partitioned into exactly two
groups, and how that partition is made doesn't matter because there
are generally exactly two different sets of rates, so for lmi it
wouldn't matter if we called them "red" and "green".

> GC> Perhaps worse, in 'ledger_invariant_init.cpp', a
> GC> 'PolicyForm' string is specified in each '.policy' file, but sometimes
> GC> it must vary by the "state" axis, so we have this ad-hockery:
> GC> 
> GC>         bool alt_form = b->database().query<bool>(DB_UsePolicyFormAlt);
> GC>         PolicyForm = p.datum(alt_form ? "PolicyFormAlternative" : 
> "PolicyForm");
> GC> 
> GC> which allows one default 'PolicyForm' and exactly one alternative.
> 
>  Now that I understand how this works, it seems kind of ingenious,
> actually.

You speak favorably of what I'm ashamed of. But maybe it's a wry
sort of praise, meaning "Clever bastardization of that field (NOT)!",
and the irony goes right over my head.

Sometimes I believe I'm right when I'm actually wrong, but here I
believe I did something wrong, and I'm quite sure I'm right about
that, and that this abomination must surely be liquidated.

> GC> I propose to rationalize those examples by introducing another level
> GC> of indirection, across the whole board:
> GC> 
> GC>  - move most strings out of '.policy' into a new file for 
> product-specific lingo
> 
>  So some strings would still remain in .policy files. What are they and how
> are they different from the strings that will be moved? I.e. why do we need
> both .policy and .lingo files, instead of just the latter?

In the original design, almost all strings used in reports were
fixed (i.e., like any purely static content in today's MST files),
and '.policy' files contained only (1) filenames (for '.database'
files etc., and actuarial-table databases) and (2) a very few
short substitutable strings like the insurance company's
postal address. We'll certainly continue using them for purpose
(1), but maybe that'll become their only purpose.

Everything else is just lingo. We want a separate abstraction
for lingo because one company's Compliance department has become
much more capric^H^H^H^H^H^Hexigent and we need a streamlined way
to keep up with their burgeoning demands.

> GC>  - look up those strings by new '.database' keys, which can vary by 
> multiple axes
> 
>  This makes sense, but I still don't see why is the extra level of
> indirection needed in the first place, i.e. why can't we just put the
> values in .lingo files directly into the .database files instead of using
> the keys for them there.

Maybe tomorrow I'll be able to figure out what std::variant is.
Unlike std::optional, it's not nullable, so it's not immediately
anathema to me. And it sounds like it's supposed to have almost
no overhead cost, which is good; but in theory the same is true
of a currency class, yet in practice, maybe not true. And it sounds
like we'd need a std::visitor to use a std::variant, which seems
like a design shortcoming.

OTOH, if it's really some wonderful thing that I just don't yet
appreciate, then maybe it's the answer to many other problems.
For example, if a currency class's datum is
  {double-dollars-and-hundredths, int64_t-cents}
then might we avoid the cost of conversion by storing both?
Well, no, I guess not, because assigning one type obliterates
the other. Maybe a std::pair of such things would be useful; or a
triple with {, string-formatted-number-with-thousands-separators}.
So you've got me thinking, but I'm not thinking clearly; and
we can discuss currency another day.

> GC> Thus, in the odd/string_db branch's microscopic example:
> GC> 
> GC>   // This can reside in a new file similar to 'dbnames.hpp':
> GC>     enum e_xyzzy
> GC>         {e_policy_form
> GC>         ,e_policy_form_KS_KY
> GC>         };
> 
>  I'm confused by this example. Isn't the whole point to have only a single
> e_policy_form which varies by e_axis_state, i.e. can have different values
> for the different states?

My intention might have been clearer if I had written:

     enum e_xyzzy
-        {e_policy_form
+        {e_policy_form_generic // for all non-extraordinary states
         ,e_policy_form_KS_KY

Thus, '.database' would contain a 1x1x1x1x1x50ishx1 array ("50ish"
because there's a federal district and some islands that regulate
insurance as though they were states):

  {e_policy_form // AL
  ,e_policy_form // AK ["Alaska" lexically follows "Alabama"]
  ,e_policy_form // AK
  ...
  ,e_policy_form // IA
  ,e_policy_form_KS_KY // KS
  ,e_policy_form_KS_KY // KY
  ,e_policy_form // LA
  ...};

and there would be a table of lingo containing:

  std::some_container<std::string> lingo =
    {...
    ,"Life-2020"
    ,"Life-2020-ABC-0123" // for KS and KY
    ...};

and I'm thinking of 'lingo' as containing "all the strings"
for every purpose, not just policy forms.

> GC>   // This can be serialized to and from a new '.lingo' file that holds 
> all such
> GC>   // proprietary lingo for all products:
> GC>     static std::map<e_xyzzy,std::string> xyzzy
> GC>         {{e_policy_form, "UL32768-NY"}
> GC>         ,{e_policy_form_KS_KY, "UL32768-X"}
> GC>         };
> GC>   // A cover function might unify this two-stage lookup:
> GC>     auto policy_form = b->database().query<e_xyzzy>(DB_L_PolicyForm);
> GC>     PolicyForm = xyzzy[policy_form];
> 
>  I.e. I am not sure how is this qualitatively different from the example
> using DB_UsePolicyFormAlt above. I must be blind, but I just don't see what
> has changed here other than using an enum instead of the bool?

It's intended to be completely different.

Code like this, in production today:
  bool alt_form = b->database().query<bool>(DB_UsePolicyFormAlt);
  PolicyForm = p.datum(alt_form ? "PolicyFormAlternative" : "PolicyForm");
is not wanted. Note carefully that "PolicyFormAlternative" and "PolicyForm"
here are not arbitrary strings like "foo" and "bar" that merely suggest
that actual values would differ--rather, they indicate these actual members:
  product_data::PolicyForm;
  product_data::PolicyFormAlternative;
and they're in double quotes because of the MemberSymbolTable thing:
  ascribe("PolicyForm"           , &product_data::PolicyForm           );
  ascribe("PolicyFormAlternative", &product_data::PolicyFormAlternative);
Adding one more alternative there would mean changing lmi code. Adding
such an alternative for another '.product'-file entity would mean
changing lmi code. That's what I want to avoid.

Instead, we want everything in tables, in external text files, which
can be changed in a text editor without altering any lmi code.

>  In particular, what would we do if we need to use UL32768-I for the states
> starting with the letter "I"? I thought the goal was to be able to add the
> entries for Idaho, Illinois, Indiana and Iowa without changing lmi sources,
> but with this approach it seems that we'd need to update them in order to
> add e_policy_form_ID_IL_IN_IA. Is this really how it's supposed to work?

In a universe parallel to lmi's lives proprietary code that generates
about a hundred different sets of '.policy', '.database',... '.strata'
files. More often than not, we find it more convenient to change that
proprietary C++ code than to edit the generated files directly, notably
because that proprietary code uses C++ inheritance to model the
relationships among products, which works quite well because new
products are most often derived from old products. I've taken care to
demonstrate the same techniques in 'dbdict.cpp' for 'sample*' products,
so that we always have a public example to discuss. Let's discuss this
code in 'master':

    // Use alternative policy form name in states beginning with "K".
    std::vector<double> alt_form(e_max_dim_state);
    alt_form[mce_s_KS] = true;
    alt_form[mce_s_KY] = true;
    Add({DB_UsePolicyFormAlt, premium_tax_dimensions, alt_form});
    Add({DB_AllowGroupQuote     , true});

along with the corresponding code in 'product_data.cpp', pretending
for purposes of this discussion that a patch like this has been applied:

--8<----8<----8<----8<----8<----8<----8<--
diff --git a/product_data.cpp b/product_data.cpp
index e908d5ed..2dd763b7 100644
--- a/product_data.cpp
+++ b/product_data.cpp
@@ -594,6 +594,8 @@ static std::string const S_DefnOutlay =
   "Outlay is premium paid out of pocket.";
 static std::string const S_DefnSpecAmt =
   "Specified amount is the nominal face amount.";
+static std::string const S_PolicyForm_sample = "UL32768-NY";
+static std::string const S_PolicyFormAlternative_sample = "UL32768-X";
 
 class sample : public product_data {public: sample();};
 
@@ -644,8 +646,8 @@ sample::sample()
     item("InsCoDomicile")              = glossed_string("WI");
 
     // Substitutable strings.
-    item("PolicyForm")                 = glossed_string("UL32768-NY");
-    item("PolicyFormAlternative")      = glossed_string("UL32768-X");
+    item("PolicyForm")                 = S_PolicyForm_sample;
+    item("PolicyFormAlternative")      = S_PolicyFormAlternative_sample;
     item("PolicyMktgName")             = glossed_string("UL Supreme");
     item("PolicyLegalName")            = glossed_string("Flexible Premium 
Adjustable Life Insurance Policy");
     item("InsCoShortName")             = glossed_string("Superior Life");
-->8---->8---->8---->8---->8---->8---->8--

[I fear to commit that without testing, but it's a throwaway example
that I don't want to make time to test.]

That patch would make the public 'sample' implementation better
resemble the proprietary sources for these policy-form entities,
so you can see a clear example of the techniques we're actually
using, proprietarily, today. Further imagine that products
derived from class sample have their own policy-form values, like

+static std::string const S_PolicyForm_sample2xyz = "UL65536-generic";
+static std::string const S_PolicyFormAlternative_sample2xyz = "UL65536-K";

for use in class sample2xyz. And, in 'master', this line:
  PolicyForm = p.datum(alt_form ? "PolicyFormAlternative" : "PolicyForm");
uses information from those '.product' and '.database' files to produce
the LedgerInvariant::PolicyForm string that we use in reports.

That leads to the discussion in 'ledger_invariant_init.cpp' on the
odd/string_db branch:

--8<----8<----8<----8<----8<----8<----8<--
// That's the old way:
//   - '.policy': contains every unique string (two in this case)
//   - '.database': indicates which of those strings to choose
//
// Here's the new way:

        enum e_xyzzy
            {e_policy_form
            ,e_policy_form_KS_KY
            };
        static std::map<e_xyzzy,std::string> xyzzy
            {{e_policy_form, "UL32768-NY"}
            ,{e_policy_form_KS_KY, "UL32768-X"}
            };
        auto policy_form = b->database().query<e_xyzzy>(DB_L_PolicyForm);
        PolicyForm = xyzzy[policy_form];
-->8---->8---->8---->8---->8---->8---->8--

You ask whether we can add
  > entries for Idaho, Illinois, Indiana and Iowa without changing lmi sources
and I intend the answer to be "yes", without changing ***lmi*** sources,
but rather by changing proprietary sources only. (That's an important
distinction: if we separate concerns in such a physically emphatic way,
then lmi needn't be recompiled, and no defect can be introduced in it.)
But does my "new way" suggestion achieve that goal?

I think it would, provided that enum e_xyzzy and map xyzzy can reside only
in the proprietary universe. (I should have said e_lingo and map lingo
for clarity.) Clearly the example above is flawed because it would place
    auto policy_form = b->database().query<e_xyzzy>(DB_L_PolicyForm);
in lmi itself, but lmi is not to know about 'e_xyzzy'. I see two ways
to repair that mistake:

(1) Let enum e_xyzzy (but not map xyzzy) reside in lmi. This actually
parallels historical practice: when we found we needed an "alternative"
policy form, we added it in lmi, and then provided specific values in
proprietary code. That required changes to ten files:
  $ grep -l PolicyFormAlt *.?pp |wc -l  
  10
which is far too much labor. OTOH, if we devise a new way that would
require only a single one-line change, hypothetically in some
'lingo.hpp' file:

   enum all_the_lingo
     {e_something
     ...
     ,e_something_else
+    ,e_some_new_thing
     ,e_another_thing

that's vastly less labor; and we could make it less likely to introduce
any defect if we assigned a value to each:
     ,e_something_else=123
+    ,e_some_new_thing=98765
     ,e_another_thing=124
This is one possible solution, but it doesn't smell even remotely right.

(2) Change the example quoted above from 'ledger_invariant_init.cpp'
(on branch odd/string_db):

-        auto policy_form = b->database().query<e_xyzzy>(DB_L_PolicyForm);
-        int policy_form = b->database().query<int>(DB_L_PolicyForm);
         PolicyForm = xyzzy[policy_form];

This is exactly what we do today for actuarial tables, e.g., in this
snippet from 'dbdict.cpp' [identical in master and odd/string_db]:

    // US 1980 CSO age last; unisex = table D.
    // Male uses table E, which is correct, as opposed to table F,
    // which contains a numerical error but was adopted by NAIC.
    int dims313[e_number_of_axes] = {3, 1, 3, 1, 1, 1, 1}; // gender, smoker
    double TgCOI[9] =
        {
         39,  37,  35, // female: sm ns us
         45,  57,  41, // male:   sm ns us
        111, 109, 107, // unisex: sm ns us
        };
    Add({DB_GuarCoiTable, e_number_of_axes, dims313, TgCOI});

There's no reason why male nonsmoker should be 57 as opposed to 12345;
that just happens to be the table index in the rate-table database.
Such indexes are fortuitous, and that's okay: hashes are deliberately
haphazard, after all.

We could still have an enumeration providing convenient tags to make
the proprietary sources clearer, but it would reside in proprietary
sources only--lmi would see only integers. We could do that for
rate tables:

-        39,  37,  35, // female: sm ns us
+        cso_1980_alb_f_sm,  cso_1980_alb_f_ns,  cso_1980_alb_f_us,

by adding an appropriate enum{cso_1980_alb_f_sm=39, ...} to the
proprietary sources only (maybe that's a good idea, independently
of what we're discussing here). And we could do exactly the same for
"lingo" strings.

So the answer to your question becomes "Yes" with this (2) change.

> GC> Do you see some better way of doing this? Here are some considerations:
> GC>  - The '.lingo' file would contain about 300 entries initially, and
> GC>    would probably never grow past about 1000. Entries might range from
> GC>    five to perhaps five thousand characters, a typical size being 100.
> GC>  - It has to be an external file, because it contains some proprietary
> GC>    lingo that must be sequestered so that lmi remains free.
> GC>  - lmi can read it from file OAOO, at startup; at run time, there will
> GC>    be no insertions or deletions.
> GC>  - Thus, it should probably be a std::unordered_map.
> 
>  There are several comments I could make here, such as that we might use
> this as an opportunity to start using something other than XML for the
> external files (as I've already mentioned a couple of times in the past,
> personally I find https://sqlite.org/aff_short.html very convincing and
> have been perfectly happy with using SQLite in other applications) and that
> we could consider finding a perfect hash for the entries if they never
> change to optimize the lookup if we decide it's worth it.

Well, OTOH, for an array
  std::vector<std::string> lingo(300);
the integers 0..299 are a perfect non-hash already, and as meaningful
(i.e., not at all) as an algorithmically-calculated hash.

>  But I'm so lost in the big picture that I think I shouldn't say anything
> at all before I understand it better. Because right now the "obvious" way
> to solve the stated goal of being able to look up strings by database_index
> seems to be to just put these strings in the .database files directly. Yet
> this doesn't seem to be considered at all and I don't know why: is it
> because we can't change the format of the .database files? Or because we
> can't (realistically) change database_entity and the code using it? BTW, if
> only one XOR the other is true, we could still change the other one, i.e.
> the structure of the external files doesn't need to map to the internal
> data structure bijectively.

I'd say it's just because I had assumed that the database_entity
implementation was so tightly bound to the notion that it could
hold only ever 'double' data without some vast change. Maybe
std::variant is the magical answer to that; I didn't consider
it because I'm unfamiliar with it.

>  Sorry for the lack of any helpful feedback, maybe I could say something
> more useful once I understand better what are we trying to do and, maybe
> even more importantly, what are we not trying to do.

No, this is tremendously helpful, because expressing the question
clearly and correctly, and making hidden assumptions explicit, is
probably more than half the battle.