Repository navigation
Conversation
GLSZM and GLRLM took the square of a zone size or run length in int and the square of a grey level in unsigned before widening, so a zone or run of 46,341 or more overflowed (undefined behaviour), and a grey level times a run length of 65,536 or more wrapped silently. The affected features were GLSZM SAE/LAE (2D, 3D and the GPU kernel) and the GLRLM run-length and grey-level emphases (2D and 3D). On a whole-organ ROI this made 3GLSZM_SAE exactly 0 and 3GLSZM_LAE negative or 142x low. Every such product is now taken in floating point: double on the CPU, the way GLDM already did it, and float in the GPU kernel, whose accumulators are float. The two #if 0 copies of the old 3D GLSZM SAE/LAE are removed. Tests, closed form at rel=1e-12, each failing without the fix: - TEST_2D_GLSZM_LARGE_ZONE_SIZE_EMPHASIS_ANALYTIC: two zones of 46,656 - TEST_3D_GLSZM_LARGE_ZONE_SIZE_EMPHASIS_ANALYTIC: two zones of 65,536 - TEST_2D_GLRLM_LONG_RUN_EMPHASIS_ANALYTIC and the 3D twin: two runs of 46,656, six run-length emphases - TEST_2D_GLRLM_HIGH_GREY_LEVEL_EMPHASIS_ANALYTIC and the 3D twin: grey level 70,000, six grey-level emphases in every direction
Registers the six new analytic tests with the coverage tooling: - the 2D GLSZM, 2D GLRLM and 3D GLRLM rows now name their analytic file in current_test - 3D GLSZM gets two vetted analytic rows at the new recipe glszm3d.analytic_two_large_zones - benchmarks.md describes the six fixtures, and config_recipes.md and the two GLSZM matrices describe the new configuration point - the four scanners read the analytic files report_output.csv/.md are regenerated.
darkclad
changed the title
… rows The 2D GLSZM, 2D GLRLM and 3D GLRLM closed-form assertions each have an oracle=analytic registry row, one per feature and recipe, as 3D GLSZM does. The five recipes they name are defined in config_recipes.md: glszm.analytic_two_large_zones, glrlm.analytic_two_long_runs, glrlm.analytic_high_grey_pair, glrlm3d.analytic_two_long_runs and glrlm3d.analytic_high_grey_pair. The mirp, pyradiomics and ibsi rows of those features describe only their own oracles again. The separator after bench_glrlm_3d_high_grey_pair in benchmarks.md has a blank line before it, so it renders as a rule rather than a heading.
2D GLSZM, 2D GLRLM and 3D GLRLM are row-scope families: each row of a feature lists in current_test every file that covers the feature. The rows of the 18 features with an analytic test, the analytic rows among them, all name the analytic file next to the oracle and regression files, so the four family scanners' --check pass.
Each of the six 32-bit range fixtures in benchmarks.md names its recipe on a Recipes: line, and the shared paragraph says each has its own recipe and analytic rows. The 3D high-grey-pair fixture, its recipe and the 3GLRLM_HGLRE analytic row say that 3GLRLM_HGLRE multiplies the level as two doubles, so for that feature the fixture pins the value without telling a 32-bit square from a double one.
The GLRLM grey-level emphases, 2D and 3D, multiply the level as `inten * inten` with `inten` declared double, and the four-factor terms square the run length as `double(j) * double(j)` rather than relying on left-to-right promotion. GLV keeps its upstream declarations. 3D GLSZM SAE and LAE sum the column sums `sj` over the zone sizes, as 2D GLSZM and 3D SZN do. One comment per file states why the squares are wide. Tests: the in-memory 3D volume runner is one template in test_main_nyxus.h, used by the GLSZM and GLRLM 3D tests, and the 3D GLRLM settings recipe is test_3d_glrlm_common.h, used by the regression and the analytic tests. The 2D GLSZM closed-form test sits under its own banner in test_all.cc. The 3D GLSZM fixture and its benchmarks.md entry say that L^2 = 2^32 is past INT_MAX, and no more.
The run-length comment sits on SRE and the grey-level comment on LGLRE, 2D and 3D, each above the term it describes. test_3d_glrlm_regression.h and test_2d_glrlm_analytic.h include only what their common header does not already bring in.
Every call to the 2D and 3D closed-form assertion helpers is wrapped in ASSERT_NO_FATAL_FAILURE, so a wrong value stops the test there rather than reporting the same defect once per direction and feature. The 3D analytic runner labels its grey-depth argument, which GLRLM does not read.
The long-run fixtures assert all four 2D directions and all thirteen 3D directions, not only the one along the line: off the line every pixel or voxel is a run of length 1, so those values and each _AVE have a closed form, and the _AVE carries the past-INT_MAX value into the mean. The twelve _AVE features get their own analytic rows in the registry. Every analytic assertion and registry row states why its band is rel=1e-12 and not SPEC 7's abs 1e-9: the values run from below 1e-9 to past 1e9. The 3D GLRLM PyRadiomics tests build their settings with make_glrlm3d_settings(), so the family has one recipe.
The 3D high-grey-pair recipe, its benchmarks.md entry and the 3GLRLM_HGLRE analytic row say that HGLRE converts the level to double before squaring it in every version of the code, rather than quoting an expression. test_3d_glrlm_pyradiomics.h includes only what test_3d_glrlm_common.h does not already bring in.
test_3d_glrlm_common.h includes feature_settings.h for Fsettings and NyxSetting rather than reaching them through test_main_nyxus.h, the PyRadiomics recipe's step comments run in order, and the GPU GLSZM comment says the zone size is squared in floating point.
vjaganat90
left a comment
vjaganat90
left a comment
There was a problem hiding this comment.
I reviewed the fix, the six analytic tests, the registry rows and the interaction with #486 and #488. The source fix is complete: every int/unsigned square that feeds SAE/LAE and the eight GLRLM emphases is now taken in floating point, in 2D and 3D, on the out-of-core paths (they share the calc functions) and in the GPU kernel. (double)a * a is used throughout, the settings refactors in the test headers are byte-identical, and no golden or assertion was dropped. I also checked the description's survey: GLDM, GLDZM and NGLDM are fine, and GLCM's IDM is as described.
Blocking
- Some of the new assertions pass without the fix under clang (inline on
test_2d_glszm_analytic.handtest_2d_glrlm_analytic.h). I builtrunAllTestsfrom this head withsrc/reverted to 3949f10 and the asserts made non-fatal. With conda-forge clang 19 at-O2on arm64, the macOS CI toolchain, the old signedj * jcompiles to a 32-bit multiply followed by an unsigned convert. That is exact for every j < 65,536, so at L = 46,656 these come out right without the fix: 2DGLSZM_LAE; 2D GLRLMSRE,SRHGLE,LRLGLEand their_AVE; 3D GLRLMSRE,LRE,SRHGLE,LRLGLEand their_AVE. Each test still fails as a whole, but the registry rows claim per-feature discrimination that doesn't hold on that leg. L = 65,536, as the 3D GLSZM fixture already uses, wraps to 0 under any compiler. - Three unrelated registry rows edited (inline on
oracle_coverage.csv:400). ffa2eb4 appends the GLSZM/GLRLM tolerance rationale toZERNIKE2Dand the twomorphology3d.analytic_lattice_solidsrows. The sentence is false for them.
Nits (inline): defect comments on most changed squares; the duplicate runner left in test_3d_gldm_common.h; r.aabb in the shared runner for #486. Two more without an anchor in the diff:
- NGTDM was missed by the survey.
int Ngp_p2 = Ngp > 1 ? Ngp * (Ngp - 1) : Ngp;(ngtdm.cpp:254,3d_ngtdm.cpp:408) is the same overflow class. It overflows at 46,342 grey levels, which a 16-bit ROI without binning can reach. Unlike GLCM, NGTDM keeps only Ng-length vectors, so nothing else fails first, andContrastthen divides by a garbage value. Please fix it here withdouble(Ngp) * (Ngp - 1), or file it next to the GLCM one and add it to the survey paragraph. - Description figures:
- The 2D high-grey test first fails on
LGLRE= NaN: the levels run 1..70,000, which includes 65,536, and65536u * 65536uis 0.HGLREis off by 2³¹ (302,516,352.5 against 2,450,000,000.5), not 4.3e9. - "Six emphases wrong" holds only where the compiler converts the overflowed
intas signed. - The rows of 30 features changed, not 18 (the
_AVEones included). - In 3D GLDM
Ndis at most 27 (26 neighbours + 1), not 26. Still bounded, so the conclusion holds. - PN-260 and PN-242 can't be looked up upstream. Please link GitHub issues.
- The 2D high-grey test first fails on
Merge order with #486 and #488
- #486: its
bin_roi_3d()callsr.aabb.require_contains()and throws on the default empty box. The newrun_3d_feature_on_volumenever sets the box, so the 3D GLSZM large-zone and 3D GLRLM analytic tests throw once both PRs are in. Git only conflicts intest_3d_glszm_common.h,test_all.ccandreport_output.*, so the GLRLM case won't show up as a conflict. Whichever lands second has to move #486's threeaabblines into this runner. - #488: its worked examples assert the overflowed values this PR removes, and
test_2d_docs_examples_mechanics.pyruns them. That coversglszm_worked_example.py:110and:113(LAE wrap andSAE= 0 on 300×300) andglrlm_worked_example.py:79-87(LRHGLE2.339e8,SRLGLE= 0,LGLRE=LRLGLE= 0). The overflow sections off_glszm.rst,f_glszm_3d.rst,f_glrlm.rstandf_glrlm_3d.rstalso describe behaviour this PR removes. I'd land this PR first and have #488 drop those pins; I'm asking for that on #488.
| // bench_glszm_2d_two_large_zones -- a 216 x 432 rectangle, the top half at intensity 1 and the | ||
| // bottom half at intensity 2. Each half is one zone of L = 216 * 216 = 46,656 pixels, and L^2 = | ||
| // 2,176,782,336 is past INT_MAX, so a size emphasis is right here only if j^2 is taken in double. | ||
| // With two zones, both of size L, the size-zone matrix has one column that matters and: | ||
| // | ||
| // SAE = (1/L^2 + 1/L^2) / 2 = 1 / L^2 LAE = (L^2 + L^2) / 2 = L^2 | ||
| // | ||
| // Read at IBSI=true, which keeps the two levels as they are. | ||
| static void make_glszm_2d_two_large_zones (std::vector<NyxusPixel>& intensity, std::vector<NyxusPixel>& mask) | ||
| { | ||
| const size_t side = 216; |
There was a problem hiding this comment.
Blocking: L = 46,656 puts L² past INT_MAX but not past UINT_MAX, and in that gap the old code isn't reliably wrong. Overflowing j * j is UB, so clang assumes the product is non-negative and converts it as unsigned. With conda-forge clang 19.1.7 at -O2 on arm64 (the macOS CI toolchain), the pre-fix calc_SAE/calc_LAE compile to a 32-bit mul followed by ucvtf, which is exact for every j < 65,536. I built runAllTests from this branch with src/ reverted to 3949f10 and the asserts made non-fatal:
GLSZM_LAEcomes out as exactlyL², so that assertion passes without the fix.GLSZM_SAEis NaN, but not because of the 46,656-pixel zones. 2D GLSZM sizes the matrix to the bounding-box area (Ns = height * width,glszm.cpp:212, 93,312 here), so the loop reaches the empty column j = 65,536, where j² wraps to 0 and 0/0 gives NaN. IfNsever becomes the largest zone, as it is in 3D, this assertion passes without the fix too.
The 3D fixture avoids this because 65,536² = 2³² wraps to 0 under either reading. Please do the same here: a 256 × 512 rectangle, two zones of 65,536. Then the old code gives SAE = inf and LAE = 0 on every compiler. The 46,656 also needs changing in the comment, benchmarks.md, config_recipes.md (glszm.analytic_two_large_zones), matrix/glszm.md and the two registry notes.
There was a problem hiding this comment.
Fixed The fixture is now a 256×512 rectangle with two zones of 65,536, and the comment, benchmarks.md, config_recipes.md, matrix/glszm.md and both registry rows follow. With src/ at the base commit, MSVC gives SAE NaN and LAE 0, and GCC wraps the same square to 0; clang 22 on x86-64 widens j * j to 64 bits and computes it exactly at any size, so no fixture catches a signed square on every compiler, and UBSan is what reports it there. Done in a93c8dc.
| // SRHGLE = LRHGLE = (1 + 4) / 2. Each _AVE is the mean of the four directions. | ||
| void test_2d_glrlm_long_run_emphasis_analytic() | ||
| { | ||
| const size_t L = 46656; |
There was a problem hiding this comment.
Blocking: This has the same gap as the 2D GLSZM fixture: below 65,536, clang compiles the old signed j * j as an unsigned 32-bit square, which is exact. On the pre-fix build (macOS arm64, conda clang 19, -O2, asserts made non-fatal), this test fails only on three emphases and their _AVE. LRE fails because P * j * j still converts signed. SRLGLE and LRHGLE fail because grey level 2 makes 4 L² wrap in unsigned. SRE, SRHGLE, LRLGLE and their _AVE come out right. In test_3d_glrlm_analytic.h:46, only SRLGLE, LRHGLE and their _AVE fail; SRE, LRE, SRHGLE and LRLGLE pass. So on that CI leg a regression in any of those lines goes through, and their registry rows ("right only if the run-length terms are taken in double") don't hold under clang. With L = 65,536 (a line of 131,072 pixels) every run-length square wraps to 0 under any compiler. That fixes both tests. The recipe, benchmark and registry text need the same change.
There was a problem hiding this comment.
Fixed Both long-run fixtures now use L = 65,536, lines of 131,072, with the recipe, benchmark and registry text to match. With src/ at the base commit all 12 features fail in each test, 2D and 3D, under MSVC. The x86-64 clang caveat from the GLSZM thread applies to the plain j * j terms. Done in a93c8dc.
| @@ -377,7 +397,7 @@ dim,feature,family,status,oracle,agreement,config_recipe,tolerance,current_test, | |||
| 2D,GABOR,gabor,vetted,skimage,agreed,gabor.python_raw_defaults,rel=1e-3,test_2d_gabor_skimage.cc,,,,in-tree,"Vetted vs scikit-image 0.26.0 at the default lists gabor_freqs=[4,16,32,64], gabor_thetas=[0,45,90,135] deg as GaborOptions::parse_input builds them (f0={4,16,32,64}, theta={0,pi/4,pi/2,3pi/4} rad) - the config the Python API always runs and a CLI run that passes both flags. All 16 pins reproduced exactly by gen_gabor_skimage.py (max |diff| 0.000e+00). This config point was added 2026-08 because the family's only vetted row covered gabor.cpp_static_defaults, a different frequency and angle set whose values differ from this one by up to 0.84 absolute on the same ROI - so no in-tree assertion covered what users compute. UNITS: the recipe is named 'python_raw' because parse_input converts the angles (deg->rad) and passes the frequencies through untouched, while nyxus.py and the CLI help both document gabor_freqs as denominators of pi and nothing in the tree divides by pi; the assertion covers the numbers that reach the filter, not the documented units contract, which is an unresolved source question (vetting report section 8 item 4). SCOPE of oracle=skimage (SPEC 4 allows one token per row; the claim is narrower than the token): skimage supplies the KERNEL at the non-degenerate (f0,theta) points - gabor_kernel(frequency=f0/2pi, sigma_x=sig2lam*2pi/f0, sigma_y=sigma_x/gamma, offset=0) cropped to the Nyxus 16x16 grid - while the count-ratio SCORE is Nyxus' own WND-CHARM definition reproduced in the generator, because skimage has no native equivalent to compare the whole feature against. A full-feature oracle would be wndcharm; not built in this tree, recorded in not_covered.md. SCOPE 2: the GPU path is NOT vetted - calculate_gpu (FFT convolution) diverges from the direct-convolution CPU path on these ROIs; the assertion covers the in-RAM CPU path only, and the GPU path's plumbing guard is TEST_NYXUS.TEST_2D_GABOR_GPU_RUNS_MECHANICS. SCOPE 3: the 16x16 kernel crop is part of the recipe rather than a detail - at gamma=0.1 only 7.9-47.6 percent of the kernel's L1 mass falls inside that window at the low frequencies, and scoring off skimage's own filtering (skimage.filters.gabor, untruncated support, zero-padded border) moves values by up to 1.005; generator part D2 re-runs that control. See audit/gabor_2d_skimage_vetting_report.md.",TEST_NYXUS.TEST_2D_GABOR_PYTHON_RAW_DEFAULTS_SKIMAGE,bench_dsb2018_2d | |||
| 2D,MEAN_FRAC,radial,regression,,disagreed,radial.shape2d_native,rel=1e-9,test_2d_radial_regression.h;test_2d_radial_invariant.h,,"cellprofiler:MeasureObjectIntensityDistribution (RadialDistribution_*) -- run 2026-08-20, does not vet",convention-mismatch,in-tree,"Not CellProfiler-vettable: CP's MeanFrac is normalised by the ROI mean intensity and lands near 1, Nyxus' is the bin's raw mean intensity. Full run, numbers and the six divergences in tests/vetting/audit/radial_2d_cellprofiler_vetting_report.md; generator tests/vetting/oracles/gen_radial_cellprofiler.py. 8-bin vector, every bin asserted separately at rel=1e-9 by TEST_2D_RADIAL_DISTRIBUTION_REGRESSION against a measured residual of 0; the raw-bin-mean convention (intensity range, and the reconstruction of the ROI total together with FRAC_AT_D) is characterized by TEST_2D_RADIAL_BIN_CONVENTIONS_REGRESSION, not asserted as an invariant, because CellProfiler's normalised MeanFrac satisfies neither. test_2d_radial_mechanics.h pins the contour frame, centre and radius the table depends on and is deliberately credited to no feature - it characterizes known defects (report §6) that a fix must change, see not_covered.md §A.1.",TEST_NYXUS.TEST_2D_RADIAL_DISTRIBUTION_REGRESSION;TEST_NYXUS.TEST_2D_RADIAL_BIN_CONVENTIONS_REGRESSION;TEST_NYXUS.TEST_2D_RADIAL_EMPTY_BINS_ARE_ZERO_INVARIANT,bench_shape8_concave_holed | |||
| 2D,RADIAL_CV,radial,regression,,disagreed,radial.shape2d_native,rel=1e-9,test_2d_radial_regression.h;test_2d_radial_invariant.h,,"cellprofiler:MeasureObjectIntensityDistribution (RadialDistribution_*) -- run 2026-08-20, does not vet",convention-mismatch,in-tree,"Not CellProfiler-vettable: CP takes the CV of the 8 wedge means over the non-empty wedges, Nyxus the CV of the 8 wedge sums over all 8. Full run, numbers and the six divergences in tests/vetting/audit/radial_2d_cellprofiler_vetting_report.md; generator tests/vetting/oracles/gen_radial_cellprofiler.py. 8-bin vector, every bin asserted separately at rel=1e-9 by TEST_2D_RADIAL_DISTRIBUTION_REGRESSION against a measured residual of 0; the sqrt(num_bins-1) bound and the empty-bin zero hold under either wedge convention and are asserted by the two invariants named in test_name. test_2d_radial_mechanics.h pins the contour frame, centre and radius the table depends on and is deliberately credited to no feature - it characterizes known defects (report §6) that a fix must change, see not_covered.md §A.1.",TEST_NYXUS.TEST_2D_RADIAL_DISTRIBUTION_REGRESSION;TEST_NYXUS.TEST_2D_RADIAL_CV_IS_WITHIN_ITS_BOUND_INVARIANT;TEST_NYXUS.TEST_2D_RADIAL_EMPTY_BINS_ARE_ZERO_INVARIANT,bench_shape8_concave_holed | |||
| 2D,ZERNIKE2D,zernike,vetted,analytic,agreed,zernike.shape2d_native,rel=1e-12,test_2d_zernike_analytic.h;test_2d_zernike_regression.h;test_2d_zernike_invariant.h;test_2d_zernike_mechanics.h,,"cellprofiler:MeasureObjectIntensityDistribution (ZernikeMagnitude) -- run 2026-08-20, different disk and normalisation, not comparable",,in-tree,"Vetted against the closed form: the factorial series for R_nm evaluated at ZernikeFeature's own geometry (unit disk on the intensity centroid, radius min(bbox w,h)). zernike.cpp uses the Singh & Walia recurrence instead, so the agreement tests the recurrence rather than restating it; centrosome's own polynomials corroborate to 4.9e-15. Measured residual 3.4e-15 worst, banded rel=1e-12 with a 1e-15 floor because (n=1,m=1) is a mathematical zero. This PR fixes a defect that made all 30 values wrong -- the angular tables were seeded at cos(theta) instead of cos(0), so every moment of repetition m used cos((m+1)*theta); see tests/vetting/audit/zernike_2d_analytic_vetting_report.md. CellProfiler's own Zernikes are NOT comparable (minimum-enclosing-circle disk, normalised by pixel count).",TEST_NYXUS.TEST_2D_ZERNIKE_MOMENTS_ANALYTIC,bench_shape8_concave_holed | |||
| 2D,ZERNIKE2D,zernike,vetted,analytic,agreed,zernike.shape2d_native,rel=1e-12,test_2d_zernike_analytic.h;test_2d_zernike_regression.h;test_2d_zernike_invariant.h;test_2d_zernike_mechanics.h,,"cellprofiler:MeasureObjectIntensityDistribution (ZernikeMagnitude) -- run 2026-08-20, different disk and normalisation, not comparable",,in-tree,"Vetted against the closed form: the factorial series for R_nm evaluated at ZernikeFeature's own geometry (unit disk on the intensity centroid, radius min(bbox w,h)). zernike.cpp uses the Singh & Walia recurrence instead, so the agreement tests the recurrence rather than restating it; centrosome's own polynomials corroborate to 4.9e-15. Measured residual 3.4e-15 worst, banded rel=1e-12 with a 1e-15 floor because (n=1,m=1) is a mathematical zero. This PR fixes a defect that made all 30 values wrong -- the angular tables were seeded at cos(theta) instead of cos(0), so every moment of repetition m used cos((m+1)*theta); see tests/vetting/audit/zernike_2d_analytic_vetting_report.md. CellProfiler's own Zernikes are NOT comparable (minimum-enclosing-circle disk, normalised by pixel count). The band is rel=1e-12 rather than SPEC 7's exact tier (abs 1e-9): these emphases run from below 1e-9 to past 1e9, so an absolute 1e-9 would accept a small one of 0 and lies below a large one's last bit.",TEST_NYXUS.TEST_2D_ZERNIKE_MOMENTS_ANALYTIC,bench_shape8_concave_holed | |||
There was a problem hiding this comment.
Blocking: ffa2eb4 appends the GLSZM/GLRLM sentence "The band is rel=1e-12 rather than SPEC 7's exact tier (abs 1e-9): these emphases run from below 1e-9 to past 1e9, so an absolute 1e-9 would accept a small one of 0 and lies below a large one's last bit." to three rows this PR doesn't touch otherwise: this ZERNIKE2D analytic row, and the 3MESH_VOLUME and 3VOLUME_CONVEXHULL morphology3d.analytic_lattice_solids rows at lines 699–700. None of them is an emphasis, and none runs past 1e9. Zernike magnitudes are at most about 1, and this row already gives its own reason, a 1e-15 floor because (n=1, m=1) is a mathematical zero, which the new sentence contradicts. The lattice-solid volumes run from 1/6 to a few thousand. Please revert those three notes and regenerate report_output.*.
There was a problem hiding this comment.
Fixed The three notes on ZERNIKE2D and the two analytic_lattice_solids rows are back to their base text, and report_output.* is regenerated. report_features.py --check and the scanners are clean. Done in a93c8dc.
| for (int j = 1; j <= Nr; j++) | ||
| { | ||
| f += P.matlab(i, j) * j * j; | ||
| f += P.matlab(i, j) * (double(j) * double(j)); |
There was a problem hiding this comment.
Nit: The repo convention (CLAUDE.md, "Annotate changed source lines") is a short comment on each changed line. Only SRE/LGLRE (GLRLM) and SAE (GLSZM) have one. These don't: LRE here, HGLRE/SRLGLE/SRHGLE/LRLGLE/LRHGLE (lines 749–879), LAE (glszm.cpp:440, 3d_glszm.cpp:838), and the 3D GLRLM twins (3d_glrlm.cpp:370, 607–702). Without a comment, (double(j) * double(j)) reads like noise that a later tidy-up could fold back into j * j. A trailing // j^2 in double: int overflows at j >= 46,341, or the grey-level equivalent, on each line would do.
There was a problem hiding this comment.
Fixed Every changed square in glrlm.cpp, 3d_glrlm.cpp, glszm.cpp and 3d_glszm.cpp now has a trailing comment naming the type it is taken in and where the integer form overflows. Done in a93c8dc.
|
|
||
| // Runs a 3D feature method on a literal volume rather than on a phantom read from disk, for any family | ||
| // whose calculate() reads the voxel cube and the voxel cloud of an LR. Lives here rather than in a family | ||
| // header because 3D tests of more than one family run it (SPEC 6.3.1). |
There was a problem hiding this comment.
Nit: The comment says this runner lives here because 3D tests of more than one family use it, but run_3d_gldm_on_volume (tests/test_3d_gldm_common.h:125-155) is still a line-for-line copy. Please make it delegate here, as run_3d_glszm_on_volume now does, so there is one runner. That also means #486's aabb change (see my comment on line 251) only has to land in one place.
There was a problem hiding this comment.
Fixed run_3d_gldm_on_volume now delegates to run_3d_feature_on_volume, so the 3D families share one runner. Done in a93c8dc.
| r.aux_max = v; | ||
| } | ||
|
|
||
| ASSERT_NO_THROW(r.initialize_fvals()); |
There was a problem hiding this comment.
Nit: This interacts with #486 at merge time. #486 has 3D GLSZM, GLRLM and GLDM calculate() place voxels through bin_roi_3d(), which reads r.aabb and throws std::out_of_range for a voxel outside the box. A default AABB is empty (xmin = INT32_MAX). #486 therefore adds r.aabb.init_*/update_* to the body of run_3d_glszm_on_volume, and this PR moves that body here. The textual conflict only shows up in test_3d_glszm_common.h. The 3D GLRLM analytic tests call this runner directly, so nothing in them conflicts, but they will throw. Whichever PR lands second needs the three aabb lines here, before initialize_fvals().
There was a problem hiding this comment.
…zero The 2D GLSZM and the 2D and 3D GLRLM long-run fixtures used zones and runs of 46,656. Its square is past INT_MAX but not past UINT_MAX, and a compiler that reads the overflowed signed product as unsigned gets it exactly, so some assertions passed without the fix. They now use 65,536, whose square 2^32 wraps to 0 in 32 bits however the product is read: the 2D GLSZM ROI is a 256x512 rectangle, the GLRLM lines are 131,072 long. With src/ at the base commit every one of the six analytic tests fails, all 12 long-run features included. A compiler may still compute a signed square exactly by widening the loop index to 64 bits, which clang 22 on x86-64 does at any size; UBSan reports the overflow there. - Registry: the GLSZM/GLRLM tolerance sentence is removed from the ZERNIKE2D analytic row and the two morphology3d.analytic_lattice_solids rows, which it does not describe. The zone and run rows, recipes, benchmarks and the GLSZM matrix describe the 65,536 fixtures. report_output.* regenerated. - Each GLSZM/GLRLM square taken in double carries a comment saying why. - run_3d_gldm_on_volume delegates to run_3d_feature_on_volume, so the 3D families share one runner, and that runner sets the ROI's bounding box for a calculate() that places voxels through it.
|
Deferred |
|
Fixed The description now gives the values measured with |
|
Answered PN-260, PN-271 and PN-242 are items on Axle's PN Jira board, https://labshare.atlassian.net/jira/software/projects/PN/boards/851, where this team tracks nyxus follow-ups; they are not GitHub issues, which is why they could not be found upstream. The description links the board. |
vjaganat90
left a comment
vjaganat90
left a comment
There was a problem hiding this comment.
LGTM. All six threads are fixed:
- I rebuilt with
src/at 3949f10 and the PR's tests, on macOS arm64 with conda clang 19 and with 21.1.8, the clang the macOS CI uses. Every one of the six analytic tests fails, with every feature the description names. The head passes. - The three unrelated registry rows match main again.
- Every changed square has a comment with the right threshold.
- The 3D families share one runner, and it sets
r.aabb.
A trial merge with #486, in either order, passes the full gtest suite with the one-line delegation.
Nits, none blocking:
- Description, compilers: clang 22 widens
j * jto 64 bits on arm64 too, not only on x86-64. With 22.1.8 at-O2, the basecalc_SAE/calc_LAEcompile to a 64-bitmulanducvtf. macOS CI is on clang 21 today, which still wraps. Once conda-forge moves to 22, though, that leg stops catching the old code, and the ASan + UBSan job is the only guard. Please say "x86-64 and arm64". - Description, Tests table: "(inf, 0 or NaN)" for the long-run test doesn't hold for
LRE_AVE,LRLGLE_AVEandLRHGLE_AVE. With the old code they come out at 0.75, 0.46875 and 1.875 in 2D (0.923, 0.577 and 2.308 in 3D): the wrapped 0 averaged with the other directions. They're still wrong, so the test fails; only the wording is off. - Description, merge order: merging #486 also conflicts in
tests/test_all.cc(keep both include lists) and intests/vetting/report_output.csv/.md(regenerate), not only in the two_common.hheaders. - cc60ec0 is an empty
ci:commit. Main lands PRs by rebase, so please drop it before merge.
…est build Merge main-binning-plumbing (PolusAI#486, tip 3f654e2) into the test-build branch that already carries PolusAI#484 and PolusAI#485, so the wheel also bins a 3D ROI voxel at the volume minimum into level 1. Conflicts resolved: - 3d_ngldm.cpp, 3d_ooc_volume.h: both sides kept. 3D NGLDM takes PolusAI#484's roi_mask_3d() and PolusAI#486's ngldm_roi_grey_level(); the header carries roi_mask_3d() and bin_roi_3d(). - test_all.cc, scan_glrlm3d_coverage.py, the first config_recipes.md hunk: both sides kept. - test_3d_glszm_common.h: PolusAI#485's run_3d_feature_on_volume() call; the helper builds the same voxel cloud and box PolusAI#486 builds inline. - test_3d_ngtdm_regression.h, matrix/ngtdm3d.md, the NGTDM config_recipes.md hunk: PolusAI#484's side, which describes the masked 3D NGTDM this tree runs. - oracle_coverage.csv, report_output.csv, report_output.md: PolusAI#486's side. The registry on this branch is not regenerated and is not authoritative. Test-build branch only; not for merging.
|
Fixed The description now says clang 22 widens |
a93c8dc to
146d1a1
Compare
2 participants
Summary
GLSZM and GLRLM squared a zone size or run length in
int, and a grey level inunsigned, before converting todouble:int, which is undefined behaviour.unsignedsilently. No sanitizer reports that.Whole-slide and whole-organ ROIs reach both routinely. On the lung CT behind the 3D binning report (3.6M voxels, largest zone about 410,000), the result was:
binCount=203GLSZM_SAE3GLSZM_LAEThe other 14 3D GLSZM features matched before and still match;
ZEdiffers only by the knownfast_log10residual.Change
Every affected product is now taken in floating point before multiplying:
doubleon the CPU, which 3D GLDM already did, andfloatin the GPU kernel, whose accumulators arefloat. Each changed square carries a comment saying why it is taken in floating point.glszm.cpp,3d_glszm.cpp,gpu/glszm.cuSAE,LAEglrlm.cpp,3d_glrlm.cppSRE,LRE,LGLRE,HGLRE,SRLGLE,SRHGLE,LRLGLE,LRHGLE3D
HGLREalready squared the grey level indouble; in3d_glrlm.cppits line is rewritten to the sameinten * intenform as the other emphases, and its analytic assertion pins the value without discriminating the fix. The two#if 0copies of the old 3D GLSZMSAE/LAEare removed.Follow-ups are tracked on Axle's PN Jira board (https://labshare.atlassian.net/jira/software/projects/PN/boards/851), which is where the PN keys below point.
Other texture families were surveyed:
double, or square a bounded index: a dependence count is at most 9 in 2D and 27 in 3D (26 neighbours plus the voxel itself).intgrey-level difference, in 2D and 3D. It overflows only past 46,341 grey levels, where the dense Ng × Ng co-occurrence matrix alone needs 17 GB. Filed as PN-260.Ngp * (Ngp - 1)(ngtdm.cpp,3d_ngtdm.cpp) is still anintproduct. It overflows at 46,342 grey levels present in the ROI, which a 16-bit ROI without binning can reach, andContrastthen divides by the wrapped value. Filed as PN-271.The GPU kernel change is compiled only in a CUDA build and was not run. That kernel accumulates every GLSZM feature in
float, so on large zones itsSAEandLAEare float-precision and are not compared against the CPU path; that is tracked in PN-242.Tests
Six closed-form (
analytic) tests at rel=1e-12, not SPEC 7's abs 1e-9, because the values run from below 1e-9 to past 1e9. The zone and run fixtures useL = 65,536, so the oldL^2is exactly 2³², which wraps to 0 in 32 bits whether the product is read signed or unsigned. The last column is what each test gets withsrc/reverted to the base commit, built with MSVC and the asserts made non-fatal:TEST_2D_GLSZM_LARGE_ZONE_SIZE_EMPHASIS_ANALYTICSAENaN,LAE0TEST_3D_GLSZM_LARGE_ZONE_SIZE_EMPHASIS_ANALYTICSAEinf,LAE0TEST_2D_GLRLM_LONG_RUN_EMPHASIS_ANALYTIC_AVELRE_AVE,LRLGLE_AVE,LRHGLE_AVE0.75, 0.46875 and 1.875, the wrapped 0 averaged with the other directionsTEST_3D_GLRLM_LONG_RUN_EMPHASIS_ANALYTIC_AVE_AVEat 0.923, 0.577 and 2.308TEST_2D_GLRLM_HIGH_GREY_LEVEL_EMPHASIS_ANALYTICLGLRE,SRLGLE,LRLGLENaN;HGLRE,SRHGLE,LRHGLE302,516,352.5 against 2,450,000,000.5 (off by 2³¹)TEST_3D_GLRLM_HIGH_GREY_LEVEL_EMPHASIS_ANALYTICLGLRE,SRLGLE,LRLGLENaN;SRHGLE,LRHGLEoff by 2³¹;HGLREalready rightThe NaNs come from the grey-level range: with no binning the level vector runs 1..70,000, which includes 65,536, and
65536u * 65536uis 0, so that empty row gives 0/0.The overflowing zone and run squares are signed, so what the old code returns depends on the compiler. GCC 15, MSVC, and clang 19 and 21 on arm64 wrap them, and every assertion above fails. clang 22, on x86-64 and arm64, widens the loop index to 64 bits and computes
j * jexactly at any size. On that compiler only the unsigned grey-level terms andLRE'sP * j * jwould come out wrong. UBSan reports the signed overflow on every compiler, so once the macOS CI moves to clang 22, the ASan + UBSan job is what guards these squares.The fix adds no refusal path, so there is no negative test.
The 3D runner that builds an ROI from a literal volume now lives in
test_main_nyxus.h. 3D GLSZM, GLRLM and GLDM all call it, and it sets the ROI's bounding box, so acalculate()that places voxels through the box can read it.The remaining commits record the tests in the vetting registry:
analyticrows, one per feature and recipe, at five new recipes:glszm.analytic_two_large_zones,glrlm.analytic_two_long_runs,glrlm.analytic_high_grey_pair,glrlm3d.analytic_two_long_runsandglrlm3d.analytic_high_grey_pair. Those families are row-scope, so every row of the 30 features involved, the_AVEones included, names the analytic file incurrent_test.analyticrows at a new recipe,glszm3d.analytic_two_large_zones.benchmarks.md,config_recipes.mdand both GLSZM matrices describe the new fixtures.report_output.*is regenerated.Merge order
bin_roi_3d()readsr.aabb. The shared 3D runner here sets the box with the same three lines fix(binning)!: honour the coarse grey depth and its sign in every family #486 adds to its runners, so the 3D GLSZM, GLRLM and GLDM tests run in either merge order. Git reports conflicts intest_3d_glszm_common.handtest_3d_gldm_common.h, resolved by this PR's one-line delegation; intests/test_all.cc, resolved by keeping both include lists; and intests/vetting/report_output.csvand.md, resolved by regenerating them.Verification
tests/python(149 passed, 4 skipped, the Arrow cases deselected), 0 diagnostics.check_test_names.py --check,check_coverage.py --check,report_features.py --checkand the GLSZM, GLRLM and GLDM scanners'--check: clean.