Re: [lmi] Group quotes, part deux

[Greg Chicares]

On 2016-05-06 18:07, Vadim Zeitlin wrote:
> On Thu, 5 May 2016 12:58:41 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> The columns to be shown in the new report depend on context, yet in a
> GC> fairly simple way: the set of columns now in HEAD are always generated,
> GC> and some (or none) are suppressed. Here, "suppressed" means that a
> GC> column doesn't appear on the report whatsoever--printing its title and
> GC> blank or all-zero contents below is not enough--and presumably I could
> GC> figure out how to accomplish that, but it's more sensible to ask for
> GC> advice.
> 
>  As long as there is no problem with still using all the columns
> internally, i.e. we don't care about the tiny overhead of maintaining them
> (and I don't think we do), this looks pretty simple as we could just modify
> wx_table_generator class to support hidden columns and wouldn't need to
> touch anything else except for the add_column() call in
> group_quote_pdf_gen_wx.cpp.
> 
> GC> I can easily identify which columns to suppress; I'm just not
> GC> sure how to prevent them from appearing in the PDF.
> 
>  For me, of course, it's the former part which is unclear while the latter
> seems pretty trivial.

Here's the logic:

    if(totals_.total(e_col_supplemental_face_amount))
        {
        suppress(e_col_supplemental_face_amount);
        suppress(e_col_total_face_amount);
        }
    if(totals_.total(e_col_additional_premium))
        {
        suppress(e_col_additional_premium);
        suppress(e_col_total_premium);
        }

I was going to insert that logic where add_column() is called, but
it appears that the headers and totals are affected, too, so could
I ask you to do that? One idea, FWIW, is to add a 'suppressed_'
member to totals_data (which might then be renamed "aggregate_data"
as you suggest elsewhere), and set that member in a "finalization"
step after all totals have been captured. But now that I re-read
your comments above, you mention changing class wx_table_generator,
so you might have a better idea already in mind that will cover the
headers and totals in a cleaner way.

> GC> Does it seem beneficial to change the way totals are calculated,
> GC> now that we'll need "averages" as well?
> 
>  IIRC you didn't want to calculate these totals in the PDF generation code
> as you wanted to avoid any possibility of rounding errors in it.

I had supposed that we were collecting numeric data in add_ledger()
and could total them later. But now I see that we're really storing
formatted strings for each row. As for totals, we really are storing
numbers, so we can calculate averages later.

> So from
> this point of view, the best solution would actually be to compute averages
> outside of it too, in a separate non-GUI piece of code which could be
> independently unit-tested. But OTOH this could well be an overkill for
> something as simple as this and I'm perfectly fine with computing the
> averages here on the fly (as you probably know, we could easily compute not
> only it but also the standard deviation on the fly too, I always use the
> same code, coming from 4.2.2 of TAoCP volume 2, to do it).

Thanks for the reminder--it's been a while since I opened that book, and
I had forgotten the technique in formula (15), though I once did know it.

In this case, though, we have at most three averages, one for each
"premium" column. The averages are to go in a row of their own below
the totals, to be labelled "Average Cost per $1000". The averages are
  1000 * premium / face_amount
for these {premium, face_amount} pairs:
  "Basic", "Basic"
  "Additional", "Supplemental" [a real problem-domain distinction]
  "Total", "Total"
There's so little to calculate here that it may as well be done on the fly.
Would you mind if I asked you to implement this? It would be nice to try
writing the PDF code myself, but I'd rather get this done sooner and then
spend time on your outstanding patches...especially since, IIRC, we're
thinking that drawing in a PDF device context isn't the best way to write
PDF files in the future so I shouldn't study it, and fitting the "average"
label looks like it might require modifying the layout.

> On Thu, 5 May 2016 15:20:12 +0000 Greg Chicares <address@hidden> wrote:
[...]
> GC> +    std::string plan_type_;
> GC> 
> GC> ...but added that member to class group_quote_pdf_generator_wx.
> GC> This keeps footnotes together in struct global_report_data, but
> GC> scatters "plan type" members across different classes. Is this
> GC> good, or bad?
> 
>  To be honest, it does seem rather bad to me as I don't understand the
> difference between it and the existing global_report_data fields, in
> particular extra_fields_. It would be more logical to me to put plan_type_
> into global_report_data as well.

All right: moved 20160508T1503Z, revision 6563.

> GC> Later, in group_quote_pdf_generator_wx::add_ledger():
> GC> 
> GC>      if(is_composite)
> GC>          {
> GC> +        plan_type_ =
> GC> +            (invar.GroupIndivSelection ? invar.GroupQuoteRubricVoluntary
> GC> +            :has_suppl_specamt_        ? invar.GroupQuoteRubricFusion
> GC> +            :                            invar.GroupQuoteRubricMandatory
> GC> +            );
> GC> +        report_data_.plan_type_footnote_ =
> GC> +            (invar.GroupIndivSelection ? invar.GroupQuoteFooterVoluntary
> GC> +            :has_suppl_specamt_        ? invar.GroupQuoteFooterFusion
> GC> +            :                            invar.GroupQuoteFooterMandatory
> GC> +            );
> GC>          report_data_.fill_global_report_data(ledger);
> GC>          }
> GC> 
> GC> It seems natural to ask whether the added lines should be moved into
> GC> global_report_data::fill_global_report_data(). But they depend on
> GC>   bool group_quote_pdf_generator_wx::has_suppl_specamt_
> GC> which isn't visible there. Of course, we could pass that boolean as
> GC> an argument,
> 
>  This looks like the best idea.
> 
> GC> or we could move its declaration into global_report_data;
> 
>  No, I agree it doesn't belong there as it's not really a "report data".

Done 20160508T2207Z, revision 6567.

I chose the way that seemed least likely to mangle your design, feeling
that a forward declaration isn't a code smell. I could have passed the
outer class's *this as an argument, or just the bool has_suppl_specamt_,
but took what seemed to be a middle road in case fill_global_report_data()
has any other use for the totals_data object. I could have rearranged the
nested classes to avoid the forward declaration, but considered the
present order more logical. Feel free to change this.

>  To finish with has_suppl_specamt_, I think it could be put into another
> struct, maybe something like aggregate_data, which would also contain the
> current running averages if we compute them here (and, of course, also the
> running totals if we change the code to compute them here too). The logic
> here would be to gather all the fields that change -- or at least can
> change with each new row -- in a separate struct. This is very far from
> critical, of course, and has_suppl_specamt_ can well continue to stay in
> the main class itself too.
> 
>  Finally, the last thing I want to say on the subject of this field is that
> personally I dislike the following line:
> 
>                 has_suppl_specamt_ = has_suppl_specamt_ || 0.0 != z;
> 
> and would rather write it as
> 
>               if (z != 0.0)
>                   {
>                   has_suppl_specamt_ = true;
>                   }

Okay: changed 20160508T1517Z, revision 6564.

Further simplified 20160508T2139Z, revision 6566.

> GC> Later, in output_document_header(), I have a different design question:
> ...
> GC> Is there good reason not to treat everything the same way, e.g.,
> GC> replacing the above block by
> GC> 
> GC>     fields.push_back(extra_summary_field({"Plan Type", 
> report_data_.product_}));
> GC> +   fields.push_back(extra_summary_field({"Product", plan_type_}));
> GC> +   fields.push_back(extra_summary_field({"Effective Date", 
> report_data_.effective_date_}));
> GC> 
> GC> and letting the loop for "parsed" input handle everything? I don't
> GC> want to stomp on your design without asking, lest I destroy something
> GC> valuable that I just don't happen to notice.
> 
>  No, I don't think this was done intentionally at all, AFAIR the extra
> summary fields were added later and I just didn't realize I could refactor
> the existing code to handle both the built-in and the extra fields in the
> same way. Looking at it right now, this definitely looks like a good idea.
> Should I do it or leave it to you as you're already in this code?

Please do it, as you're the original author so it's easier for you.
I've completed the changes I intended for now, so I'll hand this
source file over to you.

The only other change I plan before this month's code freeze is to
add a footnote, and a line in the "summary", listing which riders
have been elected (in contradistinction to the list of all riders
that are available for potential election). But that's trivial and
I'm deliberately leaving it for last.

> GC> BTW:
> GC> +   fields.push_back(extra_summary_field({"Plan Type", plan_type_}));
> GC> Is that the "best" syntax in modern C++?
> 
>  In modern C++ you would actually write
> 
>       fields.emplace_back(extra_summary_field{"Plan Type", plan_type_});

Thanks. Changed 20160508T1530Z, revision 6565.

I changed only that one line...

> and I'm not sure why I hadn't done this here.
[...]
> again, it's a mistake (even though a quite harmless one) -- should I fix it
> or should I avoid touching this code while you're working on it?

...so let me ask you to address this globally as you like along
with any other C++11-ization that seems advisable.