Tickets/dm 15203 #388
Tickets/dm 15203 #388
Conversation
src/math/Statistics.cc
Outdated
| unsigned long left = 0; // number of values less than naive | ||
| unsigned long middle = 0; // number of values equal to naive | ||
|
|
||
| for (auto ptr = begin; ptr != end; ++ptr) { |
There was a problem hiding this comment.
I'd love to, but I have begin and end not an STL container.
src/math/Statistics.cc
Outdated
| { | ||
| // investigate the cumulative histogram near naive | ||
| unsigned long left = 0; // number of values less than naive | ||
| unsigned long middle = 0; // number of values equal to naive |
There was a problem hiding this comment.
I would have used std::size_t because that's what std::vector uses, but I'm pretty sure it's a typedef for unsigned long.
src/math/Statistics.cc
Outdated
| @@ -495,15 +498,92 @@ double percentile(std::vector<Pixel> &img, double const fraction) { | |||
| } | |||
| } | |||
|
|
|||
| namespace { | |||
| // | |||
| // Helper function to estimate a floating-point quantile | |||
There was a problem hiding this comment.
Add "from integer data"?
src/math/Statistics.cc
Outdated
| } | ||
|
|
||
| /** | ||
| * @internal A wrapper using the nth_element() built-in to compute percentiles for an image |
There was a problem hiding this comment.
Why is this outside the anon namespace if it's @internal?
There was a problem hiding this comment.
Pre-existing code. I'll add it to the anon namespace
src/math/Statistics.cc
Outdated
| /** | ||
| * @internal A wrapper using the nth_element() built-in to compute percentiles for an image | ||
| * | ||
| * @param img an afw::Image |
There was a problem hiding this comment.
It's a fake kind of an image, right, because it's a std::vector?
tests/test_statistics.py
Outdated
| image.set(self.val) | ||
| # Note that the mean/median/std may not be quite equal to the requested values | ||
| self.images.append((image, | ||
| isInt, np.mean(image.array), np.median(image.array), np.std(image.array))) |
tests/test_statistics.py
Outdated
| stats = afwMath.makeStatistics(image, afwMath.MEDIAN) | ||
|
|
||
| self.assertEqual(stats.getValue(), stats.getValue(afwMath.MEDIAN)) | ||
| self.assertEqual(stats.getResult()[ | ||
| 0], stats.getResult(afwMath.MEDIAN)[0]) | ||
| self.assertEqual(stats.getResult()[0], stats.getResult(afwMath.MEDIAN)[0]) |
tests/test_statistics.py
Outdated
| stats = afwMath.makeStatistics(image, afwMath.NPOINT | afwMath.STDEV | afwMath.MEAN | afwMath.SUM) | ||
|
|
||
| self.assertEqual(stats.getValue(afwMath.NPOINT), image.getWidth()*image.getHeight()) | ||
| self.assertEqual(stats.getValue(afwMath.NPOINT)*stats.getValue(afwMath.MEAN), | ||
| stats.getValue(afwMath.SUM)) | ||
| self.assertEqual(stats.getValue(afwMath.MEAN), self.val) | ||
|
|
||
| self.assertAlmostEqual(stats.getValue(afwMath.MEAN), mean, delta=0.000004) |
There was a problem hiding this comment.
You use delta=0.000004 a lot. Should you put this particular value into a variable so if there's a need to change it later it doesn't need to be changed everywhere? Or did you tune the delta for each test?
And I think scientific notation would be easier to read.
There was a problem hiding this comment.
I thought about that, but apparently never did the work. I did check that all the tolerances that could be the same were. I'll fix this.
tests/test_statistics.py
Outdated
| self.assertEqual(stats.getValue(afwMath.MEDIAN), self.val) | ||
| for image, isInt, mean, median, std in self.images: | ||
| med = afwMath.makeStatistics(image, afwMath.MEDIAN).getValue() | ||
| self.assertAlmostEqual(med, median, delta=0.00022 if isInt else 0.00000075) |
There was a problem hiding this comment.
Why are the deltas for the integer images always significantly larger than that for the floats? More statistical noise from the quantisation?
There was a problem hiding this comment.
I think so, but didn't look very hard. I suppose I should; sigh.
tests/test_statistics.py
Outdated
| stats = afwMath.makeStatistics(img, afwMath.MEANCLIP | afwMath.NCLIPPED) | ||
| self.assertEqual(stats.getValue(afwMath.MEANCLIP), 0) | ||
| self.assertEqual(stats.getValue(afwMath.NCLIPPED), 0) | ||
| for image in self.images: |
There was a problem hiding this comment.
I don't think this is the right thing to do here. The test is checking single-pixel images, so your large images are useless, and indeed aren't used.
RobertLuptonTheGood
force-pushed
the
tickets/DM-15203
branch
from
825b474
to
aff7582
Compare
The two changes are independent, but both are needed to test the new code that treats percentiles of integer images correctly.
RobertLuptonTheGood
force-pushed
the
tickets/DM-15203
branch
from
aff7582
to
5b0f81b
Compare
No description provided.