Re: [lmi] Group quotes, part deux

[Vadim Zeitlin]

On Sun, 8 May 2016 22:16:25 +0000 Greg Chicares <address@hidden> wrote:

GC> Here's the logic:
GC> 
GC>     if(totals_.total(e_col_supplemental_face_amount))
GC>         {
GC>         suppress(e_col_supplemental_face_amount);
GC>         suppress(e_col_total_face_amount);
GC>         }
GC>     if(totals_.total(e_col_additional_premium))
GC>         {
GC>         suppress(e_col_additional_premium);
GC>         suppress(e_col_total_premium);
GC>         }

 Sorry but isn't this logic reversed? As written, it means that the columns
in question will only be shown if they contain only zeroes everywhere
(unless the supplemental face amount or additional premium can be negative
and just happen to sum up to 0?). It would seem to be more useful to hide
these columns if they don't contain anything but zeroes instead... Was this
the intentional and are there missing "== 0" above or should it really be
as written?


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

 Yes, my idea was to just allow having hidden columns at this level. I.e.
the columns are still added (almost) as usual, but some of them can be
marked as being hidden and don't appear in the output. This has the
advantage of not requiring any other changes to the code.

GC> In this case, though, we have at most three averages, one for each
GC> "premium" column. The averages are to go in a row of their own below
GC> the totals, to be labelled "Average Cost per $1000". The averages are
GC>   1000 * premium / face_amount
GC> for these {premium, face_amount} pairs:
GC>   "Basic", "Basic"
GC>   "Additional", "Supplemental" [a real problem-domain distinction]
GC>   "Total", "Total"
GC> There's so little to calculate here that it may as well be done on the fly.
GC> Would you mind if I asked you to implement this?

 This doesn't seem difficult, but I'm not sure to understand how exactly is
this supposed to look like, could you please help me with my (ASCII) mock
up? Here is how it is now:

                  ----------------------------------------------------------
                  |                                                        |
----------------------------------------------------------------------------
| Census   Totals:| $ n,nnn | $ n,nn | $ n.nn | $ n.nn | $ n,nnn | $ n,nnn |
----------------------------------------------------------------------------
|      |          | Basic   | Basic  | Suppl. | Addit. | Total   | Total   |
|      |          |  Face   | Annual |  Face  | Annual |  Face   | Annual  |
| Part#| ...      | Amount  | Premium| Amount | Premium| Amount  | Premium |
----------------------------------------------------------------------------

I understand that a row with "Average Cost per $1000" needs to be added
below the "Census ... Totals" one, but it's not clear where should the
"Census" label go. Aesthetically it would seem to be best to centre it
vertically, but this is not something that can be easily done with the
current code, so it would be simpler if this could be avoided. I'm also not
sure what, if anything, should be shown for the amount columns. Or should,
perhaps, these averages centered and span the pair of columns each too? But
how would this work if one of "Supplemental Face Amount" and "Additional
Annual Premium" columns is hidden, but the other one is not?

 It would be ideal if you could please just copy and paste my ASCII art
(surely it can be called nothing else) above with the desired changes, but
I'd also accept a thousand words of explanation -- you'll probably need at
least that many because currently I'm rather lost, even if this seems like
a completely trivial problem.


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

 I'll do this slightly later, for now I'd really like to deal with the two
questions above.

GC> >  In modern C++ you would actually write
GC> > 
GC> >   fields.emplace_back(extra_summary_field{"Plan Type", plan_type_});
GC> 
GC> Thanks. Changed 20160508T1530Z, revision 6565.
GC> 
GC> I changed only that one line...
GC> 
GC> > and I'm not sure why I hadn't done this here.
GC> [...]
GC> > again, it's a mistake (even though a quite harmless one) -- should I fix 
it
GC> > or should I avoid touching this code while you're working on it?
GC> 
GC> ...so let me ask you to address this globally as you like along
GC> with any other C++11-ization that seems advisable.

 As mentioned before, I'd prefer to modernize the entire code base at once,
as otherwise it's going to be jarring to see "NULL" here and "nullptr"
there, lmi::uncopyable<> here and "= delete" there and so on. But doing it
right now would certainly create many conflicts with the still outstanding
patches, so I'd like to wait until you can apply at least those of them
that you plan to apply.

 But, again, this is less urgent than the two questions in the beginning of
this email, thanks in advance for helping me with them,
VZ