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 ----- --
Gnutest
Description: Binary data
signature.asc
Description: Digital signature
_______________________________________________ gnucash-devel mailing list [email protected] https://lists.gnucash.org/mailman/listinfo/gnucash-devel
