Gnucash reports

[Peter Broadbery]

Hi Geert,

I've taken your suggestions and rebased the code into a new branch,
pab/topic/report-rewrite/v3.
This branch updates to current truck (I had to add a fixup commit at
the end), and renames the files you mentioned (now collectors and
report-collectors).

I think there's one more round to the final version unless anyone says
otherwise, mostly to cleanup a couple of style issues.

Peter


On Sat, May 18, 2013 at 7:08 PM, Geert Janssens
<janssens-geert at telenet.be> wrote:
> Peter,
>
>
>
> I have pulled your branch in my local repository and tested from there. I
> haven't run all the new reports, but I did play with the net worth bar
> chart. When I run it over my complete data file, the time to generate the
> report drops from 25 seconds to 5. That's a pretty nice result !
>
>
>
> I looked a bit deeper at your branch. There's not much I can say on the code
> itself. It looks ok and seems to do the job (will test some more right
> before it gets committed).
>
>
>
> I would just suggest some cleanup of the commits themselves. IMO the first 6
> commits can easily be squashed together. They all do the same thing, but for
> different reports.
>
>
>
> And at some point you add a file called reports-2.scm, which gets installed
> in the standard-reports directory. That seems to be an odd location for that
> file. I would rather expect it next to streamers.scm in
> guile-modules/gnucash/report/report-system/. But perhaps I'm completely
> misinterpreting this. Would it make sense to call the script collector.scm
> rather than the generic reports-2.scm ?
>
>
>
> And lastly: you have readded the old reports. This is useful for comparison
> but we don't need the two versions in the final code. The users will only
> want to use your improved reports.
>
>
>
> That's my feedback for now.
>
>
>
> I'm looking forward to the final result !
>
>
>
> Geert
>
>
>
> On Saturday 18 May 2013 15:23:10 Peter Broadbery wrote:
>
>> Hi Geert,
>
>>
>
>> Thanks for taking a look at the code. Status update: I'm not actively
>
>> changing anything at the moment, the only thing not committed is a
>
>> small optimisation to the way %done is calculated.
>
>>
>
>> I'll try to split the major commits and whitespace changes (apologies,
>
>> I blame emacs), and improve some of the commit messages; What else
>
>> needs to be done prior to getting this merged?
>
>>
>
>> The work so far updates the category reports (Expense/Assets/... over
>
>> time) and the net worth barchart -- these were the two bits of code
>
>> which really did badly on my data. As you've probably seen, the major
>
>> changes were:
>
>> First, ensuring that transactions are only retrieved once and then
>
>> processed - the original version was requesting data for each cell in
>
>> the report. The collector feature is probably the major piece here.
>
>> Note that scheme isn't a language I've used much, so apologies if the
>
>> style is a bit poor.. suggestions welcomed.
>
>> Secondly, adding enough testing that I could be reasonably confident
>
>> that the reports were identical to existing reports - basically unit
>
>> tests and adding temporary menu options for the original reports.
>
>> From memory, there are almost no unit tests for the scheme functions,
>
>> but adding some to the build infrastructure was relatively painless.
>
>> Testing reports is always painful, and I'm not sure that my current
>
>> approach is the ideal - reports are generated into a string, then
>
>> parsed and then tested vs. "known" values.
>
>>
>
>> Current plans are - first, get the current changes into a state where
>
>> they can be merged (providing that makes sense), and then look at more
>
>> performance issues. If some other reports are slow, I may take a look
>
>> at those too.
>
>>
>
>> Peter
>
>>
>
>>
>
>>
>
>> On Sat, May 18, 2013 at 11:32 AM, Geert Janssens
>
>>
>
>> <janssens-geert at telenet.be> wrote:
>
>> > On Saturday 30 March 2013 20:04:13 Peter Broadbery wrote:
>
>> >> Hi,
>
>> >>
>
>> >>
>
>> >>
>
>> >> I've been modifying the report code to remove the current rather bad
>
>> >>
>
>> >> quadratic behaviour it sometimes displays. Current results take a 10
>
>> >>
>
>> >> second multi-year balance report down below a second.
>
>> >>
>
>> >>
>
>> >>
>
>> >> Would there be interest in getting this work into trunk? Obviously, it
>
>> >>
>
>> >> needs further cleanup and testing, but I thought an early heads up to
>
>> >>
>
>> >> the mailing list would be useful for getting comments.
>
>> >>
>
>> >>
>
>> >>
>
>> >> There is a work in progress in
>
>> >>
>
>> >> https://github.com/pbroadbery/gnucash/tree/topic/pab/blah-reports. It
>
>> >>
>
>> >> adds new reports (prefixed with 'blah' for no good reason), so they
>
>> >>
>
>> >> can be run side by side with the existing reports. It also adds
>
>> >>
>
>> >> various simple tests that exercise both the standard reports and my
>
>> >>
>
>> >> modified versions.
>
>> >>
>
>> >>
>
>> >>
>
>> >> Regards,
>
>> >>
>
>> >>
>
>> >>
>
>> >> Peter
>
>> >>
>
>> >> _______________________________________________
>
>> >>
>
>> >> gnucash-devel mailing list
>
>> >>
>
>> >> gnucash-devel at gnucash.org
>
>> >>
>
>> >> https://lists.gnucash.org/mailman/listinfo/gnucash-devel
>
>> >
>
>> > Hi Peter,
>
>> >
>
>> >
>
>> >
>
>> > I took some time to look at your reports work.
>
>> >
>
>> >
>
>> >
>
>> > I noticed it has moved to another branch now:
>
>> >
>
>> >
>> > https://github.com/pbroadbery/gnucash/commits/pab/topic/report-rewrite/v1
>
>> >
>
>> >
>
>> >
>
>> > I only read through the commits so far, which is of course not the same
>> > as
>
>> > actually running the code. But it looks like you have some progress
>
>> > already.
>
>> >
>
>> >
>
>> >
>
>> > Can you give a short status update ? What you have done so far, what
>> > went
>
>> > well, where do you see issues, what will be next ?
>
>> >
>
>> >
>
>> >
>
>> > A minor side remark: when you rewrote the chart reports to make use of
>
>> > your
>
>> > new collector feature, you fixed lots of whitespace issues in one go. I
>
>> > think fixing these whitespace issues is a good thing. However, the way
>> > you
>
>> > did it (in the same commit that also hold the functional changes) makes
>> > it
>
>> > rather hard to see the imporant changes at a glance. When you clean up
>
>> > your
>
>> > code for inclusion, can you split up that commit in two ? One for all
>> > the
>
>> > whitespace cleanups and one for the functional changes.
>
>> >
>
>> >
>
>> >
>
>> > Thanks for your work so far !
>
>> >
>
>> >
>
>> >
>
>> > Geert