Advanced portfolio report [another version]

[Andrew Sackville-West]

On Wed, Nov 28, 2007 at 11:38:34PM -0500, Morrison J. Chang wrote:
> On Wed, 2007-11-28 at 22:46 -0500, Morrison J. Chang wrote:
> > 
> > I'll post what I've got shortly.
> 
> Okay here is the full file. This one also needs to be cleaned up for the
> log statements and comments.
> 
> I called it advanced-portfolio-test.scm so that I could run it with the
> old one.
> 
> If anything else this might be instructive to anyone looking to play
> around with scheme.

I just love scheme. I can't explain why, but I do.

> 
> Notes:
> In this version I created two new lists for holding the sold-value and
> sold-basis-value. I apply stock splits against the basis in
> basis-modify.

I've only quickly skimmed your code. ISTM that the basis manipulation
should be as contained as possible. You've broken our the handling of
splits/mergers into YANFunction, when its really not necessary. A
little work with pencil-paper tells me that you can abstract the
splits/mergers into one case with a simple two-step algorithm:

-  Figure out the ratio that results from the change in number of units
-  Apply that ratio to every cell in the basis list (recursion makes
   this trivially simple to implement for any type of basis-list)

It doesn't matter what kind of basis you're taking. I've moved the
splits/mergers code into the basis-builder function. It's really not
the job of anything *outside* the basis-builder to care one way or
another what kind of basis-changing operation is going on. A simple
check for a zero value tells basis builder whether to handle as a
buy/sell or a split/merger. 

What purpose is the soldvaluecoll?  It looks like you total it up but
never actually use it. Debug? Same with soldbasiscoll?

I've just run this report against one of my test files and it fails on
a sales txn with capital gains.  The use of (xaccAccountGetSplitList
current) (line 547 in your report, I use it too) forces iteration over
every split that connects to the account, but in a sale with gain or
loss, each txn has more than one split that touches the account. This
means that the txn gets checked more than once and it affects the
basis and other calculations once for *each* split in the txn that
touches the account. This is wrong, and doubles the impact of those
txns. My version of the report fixes this by paying attention to which
splits have already been seen in a simple list. We only examine a
split if we've not seen it before. This is at line ~415 in mine.

Look at mine too, the (cond)  at around line 460-480, much simplified
parsing of the splits as compared to yours (lines ~484-553) and the
original. There is no need to iterate through the splits multiple
times if you change the order in which they are parsed. The only time
you have to actually look at the split is when its *not* the stock
itself and then the cases break down nicely into just a couple things:
moneyin, moneyout, brokerage (ACCT-TYPE-EXPENSE) or dividend
(ACCT-TYPE-INCOME). 

There is one part of mine that is broken in this regard: a stock
purchase made directly from income -- like an employer's contribution
to an IRA that automatically purchases shares. I think yours suffers
from this as well. There is no way to differentiate between a dividend
and this other case. Probably should be written up in the docs (I
haven't read how they currently handle this case).

A
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.gnucash.org/pipermail/gnucash-devel/attachments/20071129/c71a4807/attachment.bin