248581 - range of y-axis for graphs is double the max value

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[Albert Ting]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7) Gecko/20040614 Firefox/0.8
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7) Gecko/20040614 Firefox/0.8

I noticed the y-axis for graphs is doubled the max value of any lines.
That means the height of the graph is roughly 2x taller than it needs to be, which
seems like a waste.  

My suggestion is to make the range by ~15% greater than the max value.  

Reproducible: Always
Steps to Reproduce:

Comment 1

[Albert Ting]

Attachment: suggested 2.17.7 patch

Ok, decided to dig into the charting code and see if I could do this myself.  
This patch seems to work for me!

Comment 2

[Gervase Markham [:gerv]]

I like it :-) Nice patch. I'll try and get this in soon.

Gerv

Comment 3

[Albert Ting]

Attachment: updated 2.17.7 patch

Minor tweak, does a better job aligning the max y-axis to the nearest "10's"

Comment 4

[Gervase Markham [:gerv]]

Comment on attachment 151883 [details] [diff] [review]
updated 2.17.7 patch

Albert: I can't give this a positive review. I've tried it, and it simply hangs
the webserver.

I looked at the code, and suspected that this was because of the unnecessary
sorting (you definitely don't find the max value of an array by sorting it -
that's really inefficient) but when I changed the sorting to use
Bugzilla::Util::max(), it still didn't work. I've spent an hour trying to work
out why, and I can't.

So, I'm afraid I have to put the ball back into your court. Please try with
large data sets (> 1000 datapoints) and see what you find.

Gerv

Comment 5

[Albert Ting]

Hi Grev,

Thanks for pointing out the max function, I didn't realize Bugzilla had one
so I just did a plain sort.  

In any case, I'm not sure why it's not working. My site has ~2500 datapoints
and I'm not seeing any hangs.  In case there's a difference in the modules I'm 
using, here's what I have:

Checking perl modules ...
Checking for       AppConfig (v1.52)   ok: found v1.55
Checking for             CGI (v2.93)   ok: found v2.99
Checking for    Data::Dumper (any)     ok: found v2.12
Checking for    Date::Format (v2.21)   ok: found v2.22
Checking for             DBI (v1.32)   ok: found v1.38
Checking for      DBD::mysql (v2.1010) ok: found v2.1028
Checking for      File::Spec (v0.82)   ok: found v0.86
Checking for      File::Temp (any)     ok: found v0.13
Checking for        Template (v2.08)   ok: found v2.10
Checking for      Text::Wrap (v2001.0131) ok: found v2001.0929

The following Perl modules are optional:
Checking for              GD (v1.20)   ok: found v2.11
Checking for     Chart::Base (v0.99)   ok: found v2.2
Checking for     XML::Parser (any)     ok: found v2.31
Checking for       GD::Graph (any)     ok: found v1.43
Checking for GD::Text::Align (any)     ok: found v1.18
Checking for     PatchReader (any)     ok: found v0.9.2

Checking user setup ...
Checking for    MySQL Server (v3.23.41) ok: found v4.0.15a
Checking for        GraphViz (any)     ok: found

Comment 6

[Albert Ting]

Attachment: updated 2.18rc1 patch

Enclosed is an updated patch that uses "max" instead of "sort".  Also
tweaked the range algorithm slightly.  Grev, I still don't understand
why you're seeing a hang, as this works for me with ~2300 bugs. 
I'm wonder if it's because I'm using a newer GD package?

Comment 7

[Wayne Mery (:wsmwk)]

Albert, you forgot to mark patch requesting review from Gerv.
Confirming, given there's potential patches, so this doesn't get lost.

Comment 8

[Gervase Markham [:gerv]]

Well, I figured out that the patch assumes Bugzilla::Util::max returns an array
when in fact it returns a scalar (the maximum value) but, even after fixing
that, I still can't work out why it hangs my webserver :-(

Gerv

Comment 9

[Albert Ting]

Hi Gerv,  I'm stumped why it hangs for you but not for me.  Seems like a small
patch.  Can you compare the module versions you are using and compare it with
comment #5?  I'm also using perl 5.8.0.

Comment 10

[Gervase Markham [:gerv]]

altlst: I narrowed it down to the Bugzilla::Util::max call, which seemed to be
handing in the first call to it partway through iterating over the list it was
given. I invented a "maxref" version which took a reference, and that didn't
help. So maybe there's something funny about those lists of data points.

Here are my module versions:
Checking perl modules ...
Checking for       AppConfig (v1.52)   ok: found v1.56
Checking for             CGI (v2.93)   ok: found v3.05
Checking for    Data::Dumper (any)     ok: found v2.121
Checking for    Date::Format (v2.21)   ok: found v2.22
Checking for             DBI (v1.38)   ok: found v1.42
Checking for      DBD::mysql (v2.1010) ok: found v2.9003
Checking for      File::Spec (v0.82)   ok: found v0.87
Checking for      File::Temp (any)     ok: found v0.14
Checking for        Template (v2.08)   ok: found v2.13
Checking for      Text::Wrap (v2001.0131) ok: found v2001.09291
Checking for    Mail::Mailer (v1.65)   ok: found v1.66
Checking for        Storable (any)     ok: found v2.09

The following Perl modules are optional:
Checking for              GD (v1.20)   ok: found v2.12
Checking for     Chart::Base (v1.0)    ok: found v2.3
Checking for     XML::Parser (any)     ok: found v2.34
Checking for       GD::Graph (any)     ok: found v1.43
Checking for GD::Text::Align (any)     ok: found v1.18
Checking for     PatchReader (v0.9.4)   found v0.9.2

Gerv

Comment 11

[Frédéric Buclin]

karl, have you time to look at this patch?

Comment 12

[A. Karl Kornel]

Attachment: Populate data sets script

This script takes 10 data sets (with no data) and generates enough data to display graphs for each set from 09/1998 to now.

Comment 13

[A. Karl Kornel]

Attachment: Before the Patch

This image shows what a chart will look like before the patch is applied.  The data for this chart came from attachment 202706 [details].  I pulled up the chart, clicked "Taller" twice, and clicked "Fatter" twice.

Comment 14

[A. Karl Kornel]

Attachment: After the Patch

This image shows what a chart will look like after the patch is applied.  The image was made in the same way as attachment 202708 [details].

Comment 15

[A. Karl Kornel]

Attachment: Patch v1.7

OK, I made a couple of changes from attachment 153824 [details] [diff] [review], but most of the logic involved in the calculation of the max Y value has remained unchanged.  However, I did change the way that the largest values of each line were determined.  Instead of taking a big list of data points and calling Bugzilla::Util::max, I'm now checking as I go.  In other words, for each data point read on a line, I check to see if that point is the largest.  I do a similar check for the grant total.

I tested this by using the script from attachment 202706 [details] to create 9 data sets.  Each data set had 2600 points.  I then created the image in attachment 202709 [details], a chart showing all 9 data sets (a total of 23400 data points on the chart).  As you can see, the chart displayed with no problem.

I also make a small change to the chart PNG template.  GD internally logs errors when something goes wrong, but we never displayed it.  I changed the template to display the error if the PNG data would not appear.  The end-user will still see a broken image symbol, but viewing the image data directly (through curl, for example) would give you a hopefully-useful error message.

Comment 16

[Teemu Mannermaa (:wicked)]

Comment on attachment 202723 [details] [diff] [review]
Patch v1.7

>Index: Bugzilla/Chart.pm
>===================================================================
>@@ -248,10 +250,13 @@
>     my $gt_index = $self->{'gt'} ? scalar(@{$self->{'lines'}}) : undef;
>+    $maxvals[$gt_index] = 0;

$gt_index is undefined if Grand Total is not used so this ends up generating a "chart.cgi: Use of uninitialized value in array element at Bugzilla/Chart.pm line 253." error in error log.

>+    # calculate maximum y value
>+    if ($self->{'cumulate'}) {
>+        foreach my $y (@maxvals) {
>+            $self->{'y_max_value'} += $y;
>+        }
>+    }

This doesn't look correct to me. It produces way too much empty space at the top of a cumulated chart.

>+    # align the max y value
>+    my $num_dec = int(log($self->{'y_max_value'}+1)/log(10));
>+    $num_dec = $num_dec - 1 if ($num_dec > 0);
>+    $self->{'y_max_value'} = int($self->{'y_max_value'}/(10 ** $num_dec)) *
>+                             (10 ** $num_dec) + 2*(10 ** $num_dec);

Uhh, ohh. This definedly needs some serious commenting if left as is. For example, I can't decipher what this tries to align the value to. Nearest 10?

Anyway, this doesn't seem to work for small values like 59 (result is 61). Also, maybe you should align the max value to next value that is dividable with eight so that y axis ticks are always whole number. Seeing bug count ticks with value like 7.256 isn't very nice. I recommend eight because there's always exactly eight ticks on the y axis (reports/chart.png.tmpl sets y_tick_number to 8).

Err, isn't the result of log(10) one? So that first line always divides with one? Isn't that a bit unnecessary?

>Index: template/en/default/reports/chart.png.tmpl
>===================================================================
>-  graph.plot(chart.data).png | stdout(1);
>+  graph.plot(chart.data).png or graph.error | stdout(1);

This doesn't seem to work but always output something like "ARRAY(0x92852a8)". Otherwise this seems a great idea because with it you can use debug=1 on the URL generating PNG image and hopefully see a clear error.

If you want to do this maybe you should check if has_error() is set (see GD::Graph documentation) and then output, correctly, all the error strings collected. Maybe this could be done in a separate bug anyhow..

Comment 17

[A. Karl Kornel]

(In reply to comment #16)
> >Index: Bugzilla/Chart.pm
> >@@ -248,10 +250,13 @@
> >     my $gt_index = $self->{'gt'} ? scalar(@{$self->{'lines'}}) : undef;
> >+    $maxvals[$gt_index] = 0;
> 
> $gt_index is undefined if Grand Total is not used so this ends up generating a
> "chart.cgi: Use of uninitialized value in array element at Bugzilla/Chart.pm
> line 253." error in error log.

True.  Fixed in my copy.

> If you want to do this maybe you should check if has_error() is set (see
> GD::Graph documentation) and then output, correctly, all the error strings
> collected. Maybe this could be done in a separate bug anyhow..
> 

You're right.  I'll leave this for another bug.

Comment 18

[A. Karl Kornel]

[e-mail]: ping

There are 2 comments that I'd like you to address, as you are the one who wrote this part of the patch.  If you like, you can submit a new patch (if you include the changes mentioned in comment 17), or you can give me instructions and I'll make the changes.

(From comment #16)
> (From update of attachment 202723 [details] [diff] [review] [edit])
> >Index: Bugzilla/Chart.pm
> >+    # calculate maximum y value
> >+    if ($self->{'cumulate'}) {
> >+        foreach my $y (@maxvals) {
> >+            $self->{'y_max_value'} += $y;
> >+        }
> >+    }
> 
> This doesn't look correct to me. It produces way too much empty space at the
> top of a cumulated chart.
> 
> >+    # align the max y value
> >+    my $num_dec = int(log($self->{'y_max_value'}+1)/log(10));
> >+    $num_dec = $num_dec - 1 if ($num_dec > 0);
> >+    $self->{'y_max_value'} = int($self->{'y_max_value'}/(10 ** $num_dec)) *
> >+                             (10 ** $num_dec) + 2*(10 ** $num_dec);
> 
> Uhh, ohh. This definedly needs some serious commenting if left as is. For
> example, I can't decipher what this tries to align the value to. Nearest 10?
> 
> Anyway, this doesn't seem to work for small values like 59 (result is 61).
> Also, maybe you should align the max value to next value that is dividable with
> eight so that y axis ticks are always whole number. Seeing bug count ticks with
> value like 7.256 isn't very nice. I recommend eight because there's always
> exactly eight ticks on the y axis (reports/chart.png.tmpl sets y_tick_number to
> 8).
> 
> Err, isn't the result of log(10) one? So that first line always divides with
> one? Isn't that a bit unnecessary?

Comment 19

[Albert Ting]

> (From update of attachment 202723 [details] [diff] [review] [edit])
> >Index: Bugzilla/Chart.pm
> >+    # calculate maximum y value
> >+    if ($self->{'cumulate'}) {
> >+        foreach my $y (@maxvals) {
> >+            $self->{'y_max_value'} += $y;
> >+        }
> >+    }
> 
> This doesn't look correct to me. It produces way too much empty space at the
> top of a cumulated chart.

This works for me.  For cumulate mode, you have N lines you want to add
together for a "grand total" line.  So the largest point will be the sum of
max point in each line.

Well, ok, that's not always true.  What you really want to do is for each X
index, add up the associated y-points for each line, and then determine
the largest y value.


> >+    # align the max y value
> >+    my $num_dec = int(log($self->{'y_max_value'}+1)/log(10));
> >+    $num_dec = $num_dec - 1 if ($num_dec > 0);
> >+    $self->{'y_max_value'} = int($self->{'y_max_value'}/(10 ** $num_dec)) *
> >+                             (10 ** $num_dec) + 2*(10 ** $num_dec);
> 
> Uhh, ohh. This definedly needs some serious commenting if left as is. For
> example, I can't decipher what this tries to align the value to. Nearest 10?

Regarding the algorithm, there is probably a better way.  But my main
objective was to first calculate how many digits y_max_value had, and then
subtract it by one.  Then snap to the second next "grid".  For example

    y_max_value=123
       3 digits, so num_dec=3-1=2
       round down y_max_value to 120, then add 2*10^1=20
       --> force y_max_value to be 140
    y_max_value=1234  --> force y_max_value to be 1400
    y_max_value=12345 --> force y_max_value to be 14000

Yes, log(10) should return 1.  But I was originally writing this in another
language, where log() actually meant natural log, ln().  So the Perl
version is overkill.

I can also understand making y_max_value always be divisible by 8.  That
would simplify the above algorithm.  A minor nit is to add a 1 to
y_max_value before making sure it's divisible by 8.

Comment 20

[A. Karl Kornel]

Attachment: Patch v1.8

Modification of attachment 202723 [details] [diff] [review] with respect to comment 18 and comment 19:

> > >Index: Bugzilla/Chart.pm
> > >+    # calculate maximum y value
> > >+    if ($self->{'cumulate'}) {
> > >+        foreach my $y (@maxvals) {
> > >+            $self->{'y_max_value'} += $y;
> > >+        }
> > >+    }
> > 
> > This doesn't look correct to me. It produces way too much empty space at the
> > top of a cumulated chart.
> 
> This works for me.  For cumulate mode, you have N lines you want to add
> together for a "grand total" line.  So the largest point will be the sum of
> max point in each line.
> 
> Well, ok, that's not always true.  What you really want to do is for each X
> index, add up the associated y-points for each line, and then determine
> the largest y value.

OK.  In this case, I keep a list of values, indexed by date.  For each value read out of the DB, I add the newly-read value to whatever is in the list at the specific date (might be zero, might not).  Then I take the largest value of the list as my initial max y value.

> > >+    # align the max y value
> > >+    my $num_dec = int(log($self->{'y_max_value'}+1)/log(10));
> > >+    $num_dec = $num_dec - 1 if ($num_dec > 0);
> > >+    $self->{'y_max_value'} = int($self->{'y_max_value'}/(10 ** $num_dec)) *
> > >+                             (10 ** $num_dec) + 2*(10 ** $num_dec);
> <<<snip>>>
> Yes, log(10) should return 1.  But I was originally writing this in another
> language, where log() actually meant natural log, ln().  So the Perl
> version is overkill.

Actually, log() in perl is log base e, so the log(10) is still needed.

> I can also understand making y_max_value always be divisible by 8.  That
> would simplify the above algorithm.  A minor nit is to add a 1 to
> y_max_value before making sure it's divisible by 8.
> 

Done, in a sense.  For numbers less than 100, I incrment the max, then keep pushing it up until its divisible by 8.  For numbers over 100, I make it divisible by 8*(10^something), where something >= 0, so that for larger maximums at least the lowest digit stays at zero.

Comment 21

[Teemu Mannermaa (:wicked)]

Comment on attachment 204076 [details] [diff] [review]
Patch v1.8

>Index: Chart.pm
>===================================================================
>+    # calculate maximum y value
>+    if ($self->{'cumulate'}) {
>+        $self->{'y_max_value'} = Bugzilla::Util::max(@datediff_total);

This patch was so close but looks like this generates lot's of "chart.cgi: Use of uninitialized value in numeric gt (>) at Bugzilla/Util.pm line 165." errors to the log everytime cumulate is used. Maybe @datediff_total array has some undefined values and is in need of zeros in previous loop?

>+    $self->{'y_max_value'} |= 0;
>+    $self->{'y_max_value'} = 1 if ($self->{'y_max_value'} == 0); # For log()

Nit: Maybe do "|= 1" because second line sets it to 1 anyway?

>+    #  If we have a single-digit number, set the max to 8 or 16

Nit: I don't get this comment. There's no code to handle single-digit numbers? Only one- or two-digit numbers and then longer numbers. Maybe also add a word or two about the longer numbers code logic, if at all possible. Otherwise comments in the if block itself is sufficient. :)

Also, you forgot to add the first hunk of template/en/default/reports/chart.png.tmpl changes from the last patch so this patch actually didn't do anything until I added that. :)

Comment 22

[A. Karl Kornel]

Attachment: Patch v1.9

Modification of attachment 204076 [details] [diff] [review] with respect to comment 21:

> >Index: Chart.pm
> >+    # calculate maximum y value
> >+    if ($self->{'cumulate'}) {
> >+        $self->{'y_max_value'} = Bugzilla::Util::max(@datediff_total);
> 
> This patch was so close but looks like this generates lot's of "chart.cgi: Use
> of uninitialized value in numeric gt (>) at Bugzilla/Util.pm line 165." errors
> to the log everytime cumulate is used. Maybe @datediff_total array has some
> undefined values and is in need of zeros in previous loop?

OK.  Before getting the max of @datediff_total, I go through and remove any undefined values that exist in the array.

> >+    $self->{'y_max_value'} |= 0;
> >+    $self->{'y_max_value'} = 1 if ($self->{'y_max_value'} == 0); # For log()
> 
> Nit: Maybe do "|= 1" because second line sets it to 1 anyway?

Updated.

> >+    #  If we have a single-digit number, set the max to 8 or 16
> 
> Nit: I don't get this comment. There's no code to handle single-digit numbers?
> Only one- or two-digit numbers and then longer numbers. Maybe also add a word
> or two about the longer numbers code logic, if at all possible. Otherwise
> comments in the if block itself is sufficient. :)

Yeah, that comment probably wasn't needed, as the next comment explained what's going on for numbers below 100.  For numbers above 100, I'm just referring to the comments in the code block.

> Also, you forgot to add the first hunk of
> template/en/default/reports/chart.png.tmpl changes from the last patch so this
> patch actually didn't do anything until I added that. :)

Weird.  It should be in this time.

Comment 23

[Teemu Mannermaa (:wicked)]

Comment on attachment 206712 [details] [diff] [review]
Patch v1.9

I like this change as it gives more detail to the chart by eliminating unnecessary blank space on the chart.

>Index: Bugzilla/Chart.pm
>===================================================================
>+    $self->{'y_max_value'} |= 1; # For log()

Nit: This makes sure we have 1 as default, which is good. Old code did also add 1 to the current value so that we had atleast some space at the top.

Comment 24

[A. Karl Kornel]

Checking in Bugzilla/Chart.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Chart.pm,v  <--  Chart.pm
new revision: 1.10; previous revision: 1.9
done
Checking in template/en/default/reports/chart.png.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/chart.png.tmpl,v  <--  chart.png.tmpl
new revision: 1.4; previous revision: 1.3
done