Repository navigation
Conversation
…dian - Added mean_discrete and mean_continuous functions in Maths/Mean.php - Added median_discrete and median_continuous functions in Maths/Median.php - Included explicit inline documentation and mathematical formula comments for all new functions - Implemented comprehensive PHPUnit test cases in tests/Maths/MathsTest.php to validate expected output and edge cases
darwinz
left a comment
darwinz
left a comment
There was a problem hiding this comment.
Thanks for the contribution, the formulas and docblocks are clear!
I merged this against current master locally. The tests pass, but vendor/bin/phpcs -n reports 7 errors (CI will fail), and median_discrete returns the wrong result when a value has frequency 0. The details are in the inline comments; the main items are the lint errors, the zero-frequency bug, input validation, and a few more tests.
You can check locally with composer test and vendor/bin/phpcs -n. Thanks!
| return $item['value']; | ||
| } | ||
| if ($cumulativeFrequency == $targetCF) { | ||
| $prevValue = $item['value']; |
There was a problem hiding this comment.
Bug: a value with frequency 0 that comes right after the cumulative frequency lands exactly on N/2 overwrites $prevValue, because the cumulative frequency is still equal to the target.
median_discrete([10, 15, 20], [2, 0, 2]) returns 17.5, but the data is 10, 10, 20, 20, so the median is 15. Please guard this with $item['freq'] > 0 and add this case to testMedianDiscrete.
| $f = $item['f']; | ||
| $h = $item['U'] - $item['L']; | ||
|
|
||
| if ($f == 0) continue; |
There was a problem hiding this comment.
This continue can never run: if a class reaches the target with f == 0, the previous class already reached it and returned. Please remove it. As a one-line if without braces, it's also a PSR-12 error (Inline control structures are not allowed).
| $prevCF = $cumulativeFrequency; | ||
| } | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
This line is unreachable once the total frequency is positive. If it ever were reached, returning 0 would look like a valid median. I'd throw an exception here instead.
| sort($numbers); | ||
| $length = count($numbers); | ||
|
|
||
| // Using intdiv to find the middle index corresponds to the mathematical formula m = (n+1)/2. |
There was a problem hiding this comment.
This comment is unrelated to the PR, and it's a bit misleading: intdiv($length, 2) isn't really the formula m = (n+1)/2. Could you drop this change to median()? Lines 28 and 33 also have trailing whitespace.
| * @return int|float Median of the discrete series | ||
| * @throws \Exception | ||
| */ | ||
| function median_discrete(array $values, array $frequencies): int|float |
There was a problem hiding this comment.
median() above validates its input with is_numeric and throws a clear \Exception. The new functions don't validate: non-numeric values give a raw TypeError, and negative frequencies are accepted. Please add the same kind of validation for values, bounds and frequencies (frequencies should be >= 0). This applies to all four new functions.
| } | ||
|
|
||
| $sumProduct = 0; | ||
| for ($i = 0; $i < count($values); $i++) { |
There was a problem hiding this comment.
Indexing with $values[$i] assumes list keys. mean_discrete(['a' => 10, 'b' => 20], [1, 1]) gives "Undefined array key" warnings and returns 0. Calling array_values() on the inputs at the start of each function fixes this (same for the other three functions).
|
|
||
| /** | ||
| * Calculates the mean of a discrete series. | ||
| * |
There was a problem hiding this comment.
Trailing whitespace here and on line 47. vendor/bin/phpcs -n reports 7 errors across Mean.php and Median.php, and CI will fail on them. vendor/bin/phpcbf fixes them all automatically.
| median('a', 'b', 'c'); | ||
| } | ||
|
|
||
| public function testMedianDiscrete() |
There was a problem hiding this comment.
Could you add exception tests for median_discrete and median_continuous (mismatched lengths, empty input, zero total frequency), plus a zero-total-frequency test for the mean functions? A case with unsorted input would also be good, since both median functions sort.
darwinz
left a comment
darwinz
left a comment
There was a problem hiding this comment.
Marking this as changes requested so it isn't merged before the items in my review above are addressed. Please see the inline comments there.
|
Hi @darwinz, thank you for the detailed review Bug fixes: Fixed zero-frequency bug in median_discrete by adding $item['freq'] > 0 guard Added array_values() to all four new functions to handle non-list keys Exception tests: mismatched arrays, empty input, zero total frequency — for all new functions All tests pass (composer run-script test) and vendor/bin/phpcs -n reports 0 errors |
3 participants
This Pull Request expands the statistical algorithms in the repository by adding support for Discrete and Continuous series to both the Mean and Median calculations. Previously, only the Individual series were supported.
Changes included:
Added mean_discrete and mean_continuous functions to Mean.php
Added median_discrete and median_continuous functions to Median.php
Added mathematical formulas
Implemented unit tests
All code has been formatted according to PSR-12 and strict typing has been used throughout.