Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
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 :).
Comment | File | Size | Author |
---|---|---|---|
#42 | batchapi_meaningful_percentages_05.patch | 6.18 KB | cwgordon7 |
#40 | batchapi_meaningful_percentages_04.patch | 6.19 KB | cwgordon7 |
#37 | batchapi_meaningful_percentages_03.patch | 5.99 KB | David_Rothstein |
#36 | batchapi_meaningful_percentages_02.patch | 6.06 KB | cwgordon7 |
#33 | batchapi_meaningful_percentages-1.patch | 1.27 KB | lilou |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedBumping to critical, this hurts cwgordon7 eyes, while we need him to test simpletest...
Comment #2
cwgordon7 CreditAttribution: cwgordon7 commentedPatch attached, actually, that was easy.
Comment #3
cwgordon7 CreditAttribution: cwgordon7 commentedWhoops cross posted didn't mean to bump back down :)
Comment #4
boombatower CreditAttribution: boombatower commentedMassive patch!
Comment #5
boombatower CreditAttribution: boombatower commentedComment #6
Damien Tournoud CreditAttribution: Damien Tournoud commented+1, the patch looks good, and solves a critical bug.
Do we need a test for that?
Comment #7
cwgordon7 CreditAttribution: cwgordon7 commentedI don't think it /is/ testable because simpletest browser can't run batch API.
Comment #8
Damien Tournoud CreditAttribution: Damien Tournoud commentedWell not everything can be tested via the web interface, but that's why we have Unit tests :)
Comment #9
Dries CreditAttribution: Dries commentedCommitted. :P
Comment #10
yched CreditAttribution: yched commentedSorry 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...)
Comment #11
webchickI agree with yched. It is incorrect to say that 99.5% is 100%.
Comment #12
webchickThis is just a -R of the previous patch, so marking RTBC.
Comment #13
Dries CreditAttribution: Dries commentedComment #14
cwgordon7 CreditAttribution: cwgordon7 commented.. 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?
Comment #15
yched CreditAttribution: yched commentedI have no pb with that :-)
Comment #16
webchickHow about round it to a tenth of a percentage. That would solve both use cases, no? :P
Comment #17
cwgordon7 CreditAttribution: cwgordon7 commentedIf we're going to a tenth of a percent, we might as well go to a hundreth of a percentage... that looks better anyway.
Comment #18
cwgordon7 CreditAttribution: cwgordon7 commentedHere's a new patch.
Comment #19
cwgordon7 CreditAttribution: cwgordon7 commentedNew patch.
Comment #20
Damien Tournoud CreditAttribution: Damien Tournoud commentedHum.
This should be
!$total
, right?Here you are comparing two floats for equality. That's bad practice.
Why not using tenths of percent internally?
Comment #21
dmitrig01 CreditAttribution: dmitrig01 commentedThis is /definitely/ not critical.
Comment #22
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #23
cwgordon7 CreditAttribution: cwgordon7 commentedAs per #20.
Comment #24
cwgordon7 CreditAttribution: cwgordon7 commented.. http://cwgordon.com/sites/default/files/batchapi_use_round_not_floor_cuz...
Comment #25
cwgordon7 CreditAttribution: cwgordon7 commentedbump?
Comment #26
David_Rothstein CreditAttribution: David_Rothstein commentedHm, 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 ;)
Comment #27
boombatower CreditAttribution: boombatower commented*loves sig figs*
Comment #28
cwgordon7 CreditAttribution: cwgordon7 commentedThat 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.
Comment #29
David_Rothstein CreditAttribution: David_Rothstein commentedTechnically 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 ;)
Comment #30
boombatower CreditAttribution: boombatower commentedEliminated 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.
Comment #31
David_Rothstein CreditAttribution: David_Rothstein commented*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).
Comment #32
cwgordon7 CreditAttribution: cwgordon7 commentedWhy not just do:
?
Comment #33
lilou CreditAttribution: lilou commentedReroll according to #32.
Comment #34
cwgordon7 CreditAttribution: cwgordon7 commentedAwesome!
Comment #35
webchickThe 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.
Comment #36
cwgordon7 CreditAttribution: cwgordon7 commentedHere'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.
Comment #37
David_Rothstein CreditAttribution: David_Rothstein commentedNice! 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:
to this:
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.
Comment #38
cwgordon7 CreditAttribution: cwgordon7 commentedLooks good, thanks David!
Comment #39
catchNot one line, Druplicon will be sad.
And there's no phpdoc for the class.
Comment #40
cwgordon7 CreditAttribution: cwgordon7 commentedI'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.
Comment #41
catchThe 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...
Comment #42
cwgordon7 CreditAttribution: cwgordon7 commentedTo avoid the controversy, I've removed the "properly" so that it's less than 80 characters and consequently fits on one line.
Comment #43
catchComment #44
Dries CreditAttribution: Dries commentedExcellent, and with tests! Committed to CVS HEAD. Thanks.