Advanced portfolio report [another version]

[Andrew Sackville-West]

On Thu, Nov 29, 2007 at 11:00:59PM -0500, Morrison J. Chang wrote:
> On Thu, 2007-11-29 at 13:34 -0800, Andrew Sackville-West wrote:
> > On Wed, Nov 28, 2007 at 11:38:34PM -0500, Morrison J. Chang wrote:
> > > 
> > > 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. 
> 
> For a while I was thinking of arguing for a modularization of the basis
> math. Frankly if the function is bigger than a page, then I have to be
> really mindful of the paren matching (yes I use emacs but
still....).

I don't disagree with that sentiment, but to me its just so logical
that all the basis handling be done in one place -- at least relative
to the rest of the code. Its certainly reasonable to pull out
different parts of the basis code into helper functions, but for *my*
personal taste, anytime you hit a basis-effecting txn-split (to
differentiate from a stock-split ;) it should call to the same
function and let that function decide how to handle it. But that's my
taste :)

For paren matching, I've become a *big* fan of Alt-( ... helps a lot.

> That and the fact that I was playing around with the map routine.
> 
> As an aside do you know if map or tail-recursive car/cdr is more
> efficient?

I *think* that is really interpreter dependent. My understanding is
that for all reasonable real-world applications properly implemented
tail-recursion is as efficient as standard iteration. A good
interpreter will be aware of the recursion and *not* actually nest
(heading into murky territory here, sorry for the vague terminology)
the calls, but just stack up the results so that you don't end up with
the massive out of contorl monster. This I gather from reading some
docs on Haskell lately. I don't know how it applies to
Scheme/Guile. Maybe someone else can chime in with info on this.

Personally, I think recursion makes for pretty code, but I'm funny that way.

...

> 
> > I've just run this report against one of my test files and it fails on
> > a sales txn with capital gains.
> 
> Odd, I thought I had it. Can you send me the test file?

attached. I've been mucking around in it a bit, but it should still
work for this use.

> 
> > 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). 
> 
> Actually I never understood why the moneyin/moneyout stuff was done in
> its own loop (maybe currency issues?). I think that is where I was
> headed with soldvaluecoll and soldbasiscoll to just get rid of it since
> the basis calc is done in the earlier loop. So in this regard the fact
> that I'm able to generate what I think are correct results with my code
> puzzles me a bit.

well you never actually use those to sold* collectors. I don't know
why the old function looped the way it did either. I wrote the current
basis-builder stuff that's in there without really looking at the rest
of the report, it may be that it could have been removed at that
point. I don't know, but it's certainly not necessary in my mind.

This is fun! Thanks for participating in this discussion. I look
forward to more. 

A





----- End forwarded message -----

-- 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Gnutest
Type: application/octet-stream
Size: 3220 bytes
Desc: not available
Url : http://lists.gnucash.org/pipermail/gnucash-devel/attachments/20071129/7ba2c039/attachment.obj 
-------------- 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/7ba2c039/attachment.bin