For example, when I run 8 simpletests and 3 are finished, it should be 37.5% done, which should round up to 38%. However, it rounds down to 37%. This odd behavior is quite disconcerting for the average user. Patch coming as soon as I have time if this huge bug isn't fixed by the time I'm done with exams :).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Priority: Normal » Critical

Bumping to critical, this hurts cwgordon7 eyes, while we need him to test simpletest...

cwgordon7’s picture

Assigned: Unassigned » cwgordon7
Priority: Critical » Normal
Status: Active » Needs review
FileSize
601 bytes

Patch attached, actually, that was easy.

cwgordon7’s picture

Priority: Normal » Critical

Whoops cross posted didn't mean to bump back down :)

boombatower’s picture

Massive patch!

boombatower’s picture

Status: Needs review » Reviewed & tested by the community
Damien Tournoud’s picture

+1, the patch looks good, and solves a critical bug.

Do we need a test for that?

cwgordon7’s picture

I don't think it /is/ testable because simpletest browser can't run batch API.

Damien Tournoud’s picture

Well not everything can be tested via the web interface, but that's why we have Unit tests :)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed. :P

yched’s picture

Status: Fixed » Active

Sorry to reopen, but I think this should be reverted - batch API *should* round down.
If you have 1000 operations and 995 are done, you want it reported as 99%, not 100%, because 100 means the whole batch is processed, and the batch engine skips to the 'finished' step.
Besides, the remaining 5 operations could still take a few minutes to run, and you let the user wait in front of a 100% progress bar, asking himself 'WTF ?'.

(edit : better numerical example...)

webchick’s picture

I agree with yched. It is incorrect to say that 99.5% is 100%.

webchick’s picture

Status: Active » Reviewed & tested by the community

This is just a -R of the previous patch, so marking RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed
cwgordon7’s picture

.. if 99.5->100% is a special case, let's make it a special case and round down there, but 37.5% should still round up to 38%, right?

yched’s picture

I have no pb with that :-)

webchick’s picture

How about round it to a tenth of a percentage. That would solve both use cases, no? :P

cwgordon7’s picture

If we're going to a tenth of a percent, we might as well go to a hundreth of a percentage... that looks better anyway.

cwgordon7’s picture

Status: Fixed » Needs review
FileSize
678 bytes

Here's a new patch.

cwgordon7’s picture

Damien Tournoud’s picture

Hum.

+    if ($total) {
+      $percentage = 100;
+    }

This should be !$total, right?

+    else {
+      // We round to the nearest hundreth.
+      // However, if it still rounds up to 100, we round down to 99.99.
+      $rounded = round($current / $total * 100, 2);
+      $percentage = ($rounded == 100) ? $rounded : 99.99;
+    }

Here you are comparing two floats for equality. That's bad practice.

Why not using tenths of percent internally?

dmitrig01’s picture

Assigned: cwgordon7 » dmitrig01

This is /definitely/ not critical.

Damien Tournoud’s picture

Assigned: dmitrig01 » Unassigned
Status: Needs review » Needs work
cwgordon7’s picture

Assigned: Unassigned » cwgordon7
Status: Needs work » Needs review

As per #20.

cwgordon7’s picture

cwgordon7’s picture

bump?

David_Rothstein’s picture

Hm, critical? Heh heh, well if it's really that important, we might as well get it right for any number of operations, which would basically mean worshiping at the altar of http://en.wikipedia.org/wiki/Significant_figures ...

The attached patch is based on @cwgoordon7's idea but should give a meaningful number of digits in the percentage regardless of how many operations you're running: 10, 100, 1000, even 32 billion. The last one isn't tested, though ;)

boombatower’s picture

*loves sig figs*

cwgordon7’s picture

That approach to significant figures is incorrect (trust me, I just had a chemistry exam a week ago). Since we are working with actual numbers here, there is an infinite degree of accuracy, and thus an infinite number of significant figures, meaning we should not be rounding at all. We don't actually have "3." tasks finished out of "8." total, we have 3.000000000000... tasks finished out of 8.000000000000 total. Therefore instead of rounding to 38% in this case, we should not be rounding at all, and leaving our answer at 37.5000000.... % (still with an infinite number of significant figures).

So it is completely wrong to use significant figures here, because that counteracts what we're trying to do. It simply is not feasible to display an infinite number of 6's on the batch progress page if we have 2 out of 3 done.

I believe my patch in #24 still stands.

David_Rothstein’s picture

Technically speaking, you're correct, and you therefore deserve an A on your chemistry exam ;)

I was using the term "significant figures" kind of loosely, though.. what I really meant was "meaningful figures", I guess? Since the number of tasks completed is always an integer and increments by 1 at a time, that's what sets the significance. In other words, if you have 3 out of 8 tasks finished = 37.500000000...%, those extra zeroes are pretty extraneous, because the next time the page refreshes you will see (at least) 4 out of 8 = 50.00000000...%. Showing 38% and 50% is more than good enough to give you the information you need. Using 37.50% and 50.00% looks a little strange, in my opinion; the first decimal place doesn't serve any purpose unless the number of tasks you're working with is above 100 (exactly how far above is something that could be the source of endless philosophical debate ;), and the second decimal place becomes useful somewhere above 1000, etc.

So I like my little approach because it's a bit more scalable and it handles the "don't round up to 100%" thing more naturally without any special cases in the code, but probably this isn't earth-shattering either way. Time to get back to work ;)

boombatower’s picture

Eliminated special cases wherever possible is a plus in my opinion. This approach makes sense since it uses an algorithm for determining the number of significant (or meaningful) figures necessary to alleviate the roundup issue.

Obviously from the scientific standpoint it doesn't, but this isn't science. ;)

I'm for this approach.

David_Rothstein’s picture

*grin* The ironic thing here is that I actually have a Ph.D. in science ;) You can draw your own conclusions from that, I suppose....

In order that my embarrassing misuse of the term "significant digits" is not enshrined in Drupal forever, here is a new version that renames the variable to $meaningful_digits. It also uses a cleaner algorithm and sets the cutoff for adding new decimal places at 200, 2000, 20000, etc (which is the maximum possible before we run into the rounding-up-to-100% problem).

cwgordon7’s picture

$decimal_places = $meaningful_digits < 2 ? 0 : $meaningful_digits - 2;

Why not just do:

$decimal_places = max($meaningful_digits - 2, 0);

?

lilou’s picture

Reroll according to #32.

cwgordon7’s picture

Status: Needs review » Reviewed & tested by the community

Awesome!

webchick’s picture

Status: Reviewed & tested by the community » Needs work

The patch seems reasonable (and is even well-commented: bonus!); however, it seems like the kind of thing that someone 2 years from now is going to stumble across and rip out because they're trying to simplify the code. I think it would be less likely to happen if we had some automated tests to prove that this should work this way.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
6.06 KB

Here's a revised patch with tests. For the sake of simplicity in testing, I moved the logic into its own function so I wouldn't have to mess around with testing Batch API, which would turn out to be very hard and very ugly.

David_Rothstein’s picture

Nice! I remember also getting scared away by the thought of having to test the whole Batch API, but your way is much much better...

I reviewed the patch and it looks great to me. In the attached version, I made a few minor fixes to code comments and such, and also, in the original code, I changed this:

$meaningful_digits = floor(log10($total / 2.0)) + 1;
$decimal_places = max($meaningful_digits - 2, 0);

to this:

$decimal_places = max(0, floor(log10($total / 2.0)) - 1);

since it saves a line of code and I think is actually easier for people to understand, in combination with the code comment that is there.

I think this should probably be RTBC.

cwgordon7’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks David!

catch’s picture

Status: Reviewed & tested by the community » Needs work
 /**
+ * Helper function for _batch_process(): returns the properly formatted
+ * percentage.

Not one line, Druplicon will be sad.

And there's no phpdoc for the class.

cwgordon7’s picture

I'm confused about the comment, I thought comments shouldn't exceed 80 characters on a line?

And I've added basic phpdoc for the class.

catch’s picture

The first line of the comment should either be less then 80 characters. Or less than 80 characters, with an extra summary underneath. But having typed that, I'm not sure if the problem is when it's two sentences, or if one long sentence is OK...

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
6.18 KB

To avoid the controversy, I've removed the "properly" so that it's less than 80 characters and consequently fits on one line.

catch’s picture

Status: Needs review » Reviewed & tested by the community
Dries’s picture

Status: Reviewed & tested by the community » Fixed

Excellent, and with tests! Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.