Repository navigation
docs: bring the documentation in line with the code - #488
Conversation
…I on warnings conf.py imports the Python package from the source tree and mocks nyxus.backend, so the docs build without compiling Nyxus. The site uses pydata-sphinx-theme; the page title carries no version number, and |release| (quoted by the citation) is read from CITATION.cff, which release-please now updates. A docs workflow builds the site with warnings as errors and checks links. The docs dependencies move from environment.yml to docs/requirements.txt, which the Read the Docs configuration uses. test_docs_conf_mechanics.py checks the release, the title and that |release| is still used.
The README becomes a landing page: what Nyxus is, how to install it, three quick starts that run as written, links into the site, the citation and the license, with absolute links so the PyPI page renders them. The site gets a card landing page with the citation, an installation page (packages, platforms, GPU packages, building from source with the CMake options), the user and developer guide section pages, and a release notes page. Package facts describe what is published now; release gaps on the PyPI and conda-forge builds are stated where they matter.
…pages docs/tools/gen_cli_reference.py renders docs/source/cli_reference.rst from docs/cli_options.toml, where each option records its default, the source line that assigns it, its values, its effect and the code it was checked against. Its --check mode fails when the TOML and the option header disagree, a default does not match its source line, or a citation is stale (test_docs_cli_reference_mechanics.py). The command-line and examples page and the WIPP page are rewritten around examples that run; plugin.json offers the arrowipc output type the program accepts.
…onventions Supported formats lists what each reader takes (TIFF, OME-TIFF, OME-Zarr, NIfTI, DICOM), how intensities become grey levels and which builds include which reader. The output page describes the columns, CSV, Arrow IPC and Parquet, and missing values. The conventions page collects what every family shares: the ROI and its bounding box, grey-level binning, 2D and 3D defaults, background, the intensity domain, units and anisotropy, constant ROIs and the entropy logarithm.
The large-images page explains the RAM limit and out-of-core processing: which families match the in-memory values on an image read as one tile, which raise an error or return wrong values on an image read as several tiles, and which inputs are refused. The volumetric page covers 3D input, anisotropy and physical spacing, whole-volume mode and 3D streaming. The GPU page lists the families computed on the GPU, how the GPU is selected from Python and the command line, and where GPU values differ from the CPU.
The IBSI mode page says what --ibsi changes, family by family. The image quality page describes the ImageQuality class and its group. The validation page explains how features are checked against independent implementations and what the coverage registry records. The troubleshooting page quotes each common error as Nyxus prints it, from Python and the command line, with its cause, and covers performance.
…indexes docs/tools/gen_feature_reference.py reads the feature registry from the code (docs/tools/feature_registry.py) and renders the feature list, the feature groups and one index per family from docs/feature_descriptions.toml (a one-line description of every code) and docs/feature_extras.toml (names, IBSI identifiers and worked-example values). The feature list shows each feature's validation status from tests/vetting/oracle_coverage.csv. Its --check mode fails on a missing or empty description, an unclaimed feature class, a stale generated file or a page whose feature counts are out of date (test_docs_feature_reference_mechanics.py).
test_2d_docs_examples_mechanics.py runs each script in docs/examples in a fresh directory and fails on a non-zero exit. Worked-example scripts assert every value their page quotes against docs/feature_extras.toml, which the harness hands them as JSON, so the scripts run on Python 3.10 without tomllib. check_test_names.py registers the dimension-agnostic docs tests.
… pages Each page follows the family template: how the values are computed, a worked example whose values a script checks, notation, settings with their real effect, output columns, the generated index, one entry per feature with the formula as the code computes it, possible issues with measured values, validation and references. The IBSI intensity histogram page is new. Known defects are stated as current behaviour with "a fix is coming".
…l pages The morphology page covers its families (basic morphology, contour, convex hull, ellipse, circles, Feret, Martin, Nassenstein, chords, Euler, extrema, geodetic, ROI radius, neighbours) in the family template; erosion, fractal dimension and the radial distribution (new) get their own pages, each with a checked worked example. Images the new pages no longer use are removed. The comments of the scikit-image erosion oracle and its generator now describe the structuring element and the counting convention Nyxus uses.
The shape and intensity moments, the Zernike moments and the Gabor features get pages in the family template with checked worked examples. The Gabor figure is regenerated by docs/tools/gen_gabor_figure.py from the parameters the code uses.
The three 2D texture families get pages in the family template, with checked worked examples, their settings (angles, offset, grey depth, IBSI mode) and their differences from IBSI and PyRadiomics measured.
The four remaining 2D texture families get pages in the family template, with checked worked examples, their neighbourhood and zone definitions, and their differences from IBSI and PyRadiomics measured.
New pages for the 3D intensity and shape families and the seven 3D texture families (GLCM, GLRLM, GLSZM, GLDM, GLDZM, NGLDM, NGTDM), in the family template with checked worked examples: how each differs from its 2D counterpart, which settings and metaparameters apply, anisotropy and out-of-core behaviour, and validation against MIRP, PyRadiomics and IBSI.
…them against the code The docstrings of the nyxus module are rewritten in numpydoc style and corrected against the code: defaults, accepted values, group names, case sensitivity, the memory limit, anisotropy, GPU selection, Raises sections and return values. The API reference pages render them. test_docstring_examples_mechanics.py runs every docstring example and test_api_arguments_mechanics.py checks the documented argument errors. Docstring-only: the docstring-stripped syntax trees of nyxus.py and functions.py are unchanged.
…otes The developer guide is rewritten for the current architecture: the run flow, adding a feature or an option, settings and metaparameters, the dispatch to in-memory, out-of-core and GPU paths, testing and vetting, CI, the documentation tooling and releases. CONTRIBUTING.md is new; the contributor notes and the vetting README, SPEC, MIGRATION and TOOLS are corrected where they disagreed with the code, and every build recipe uses build-cli with an explicit Release build. The pipeline diagram is redrawn to match the code, and a coverage-registry row and the fixture READMEs are corrected.
34fbc3c to
b141226
Compare
…mples Two worked examples quote the effect of an overflow that is undefined behaviour in C++, and the arm64 and macOS CI builds give another result than x86-64: - fractal dimension: an intensity of 4294967295 plus 1, converted from double to a 32-bit unsigned integer, is 0 on x86-64 (the pixel is dropped from the box count) and saturates on ARM (the pixel is counted). The script expects the x86-64 value only on x86-64. - GLSZM large area emphasis on a 250 x 250 zone: the 32-bit signed square wraps on most builds, and one macOS ARM build returned the true value. The script accepts either. Both pages now say that the value depends on the build.
… macOS The macOS ARM test build squares the zone size in unsigned 32-bit arithmetic: on the 300 x 300 zone it returns (89999^2 mod 2^32 + 1) / 2 = 1 902 426 353, and on the 250 x 250 zone the square still fits, so it returns the true 1 953 062 501. The example now accepts the signed wrap quoted on the page or the unsigned wrap computed from the zone size, and the page names both values.
vjaganat90
left a comment
There was a problem hiding this comment.
I went through all 186 files and checked the pages against the code: formulas, defaults, settings, column names, IBSI ids, Validation claims against oracle_coverage.csv, and the guide's CLI, CMake, format, GPU, out-of-core and output claims. I re-derived a sample of worked-example values with numpy/scipy, and ran the 2D and 3D worked examples on builds of main, #485 and #486. Most of it holds up. This is what I checked and found right:
- Generators: both generators'
--checkreally catch drift in names, defaults and staleness, and their tests mutate a default, a cited file and a missingdefault_atto prove it. plugin.json: the removed feature entries were all invalid on main, and the remaining ones are valid.- Build config: the release-please
x-release-please-versionmarker works inCITATION.cff, andautodoc_mock_importsmatches.readthedocs.yml. - Python API docstrings: the signatures, defaults, group names, Raises sections, ROI blacklist syntax,
Nested,ImageQuality, anisotropy andram_limitall match the code. - 3D pages: the formulas, index example values, IBSI ids and Validation claims re-derive and match the registry, apart from the comments below.
- Guide pages: they match main, apart from the comments below.
Blocking
-
Executed examples assert undefined behaviour.
test_2d_docs_examples_mechanics.pyruns inpytest tests/python, so every value a script asserts becomes a CI contract. Three blocks assert the result of UB:fractal_worked_example.py:99-105keys an out-of-rangedouble→uint32conversion onplatform.machine().glszm_worked_example.py:102-116pins a signedintsquare that wraps. It has already needed two follow-up commits to accept other wraps.f_glszm_3d.rst:456-465quotes a 3D LAE wrap that macOS main doesn't produce.
The compiler decides these, not the architecture. Please drop those pins and describe the overflow as a defect.
-
Merge order with #485 and #486. These pages describe main correctly, but both open PRs change what they describe, and the docs checks make that a CI failure, not just stale text:
- Examples fail. With #485,
glszm_worked_example.pyandglrlm_worked_example.pyfail. With #486,directory_and_params.py(set_params(coarse_gray_depth=256)now raises) and theglrlm_3d,glszm_3d,gldm_3dandngtdm_3dworked examples fail. --checkfails.gen_feature_reference.py --checkfails on either PR's registry rows.gen_cli_reference.py --checkfails on #486's three new includes inenvironment.cpp(point 3).- Text goes stale with no failure. On the NGLDM/GLDZM/GLCM pages,
conventions.rst,cli_reference.rstand thenyxus.pydocstrings, nothing fails and the text just goes stale.
I'd land #485 and #486 first and rebase this on them. The rebase would regenerate the indexes, replace the overflow and binning sections with the fixed behaviour, and pin the new values. The per-page list is at the end of this review.
- Examples fail. With #485,
-
cli_options.tomlcites defaults by line number (inline). Any edit above a cited line in about 25 C++ files fails the Python suite. -
The description doesn't match the branch in four places:
- "The code is unchanged": three
ValueErrormessages innyxus.pychanged. - "Each worked-example script asserts every value its page quotes":
glszm_3d,glrlm_3d,ngldmandfractaldon't. - "Each page's Validation section claims only the oracles in the coverage registry": the 2D GLDM, GLDZM, GLSZM, NGLDM and NGTDM pages cite IBSI consensus checks that have no
ibsirow.AREA_UM2is listed as MATLAB-validated but is 0 in every real run.f_gldzm_3d.rstcites regression tests that don't exist. - The
CLAUDE.md/CONTRIBUTING.mdchange to the defect-annotation rule is a policy change, not a correction (inline).
- "The code is unchanged": three
-
EROSIONS_2_VANISHstaysvettedafter this PR's own rewritten note says the oracle can't tell the two conventions apart (inline). -
test_2d_docs_examples_mechanics.pyruns 3D and dimension-free scripts under a2dname (SPEC 6.1; inline). -
Statements that are wrong on main (inline):
- the GLCM values-vs-positions list under PyRadiomics binning;
- the GLRLM overflow table, which leaves out
LRE; coarse_gray_depth/ibsi, which don't say thatibsi=Trueturns texture binning off;- the
set_paramsnote (HISTOGRAMdoes change); - the holed square's inner loop, which is 13.66, not 16;
- the whole-slide fractal value of a filled rectangle;
- the
POWER_SPECTRUM_SLOPEformula, which leaves out an out-of-bounds read; - Gabor kernel size 1, which the page calls valid but which reads past the buffer.
Code defects written up as behaviour
Checking the pages against the code turned up about 20 real defects. The pages describe them carefully, but as conventions, and the scripts pin many of them, so each fix will fail the docs suite as if it were a regression. The inline comments give each cause and its fix:
- Intensity and histograms:
HISTOGRAMalone gives zero columns;ENERGY/ROOT_MEAN_SQUAREDwrap at 32 bits, in 2D and 3D; theIH_MAX_GRADIENTDBL_MINseed; IH bin-edge rounding;--coarseGrayDepth=0writes before the histogram buffer. - Box and mode: whole-image and whole-volume boxes are one pixel too large (
aabb.hinit_from_wh/init_from_whd); 3D shape undersingle_roi=Trueuses the bounding box. - Morphology: ellipse, circle and hexagonality codes are wrong when requested alone;
AREA_UM2is always 0; the circle/ROI-radius contour offset; Feret samples both 0° and 180°. - Erosion: the asymmetric frozen border; the constant-ROI skip of a mask-only feature.
- Zernike and Gabor: Zernike drops the centre pixel; Gabor's CPU and GPU offsets differ, frequency and angle are swapped, and kernel size 1 reads past the buffer.
- Image quality and out-of-core: image quality reads uninitialised members out of core; the out-of-core contour offset in moments and radial distribution.
- Texture: GLCM's per-position overwrite of
DIFAVE/SUMAVERAGE/IV; GLCM's constant ROI ignores--noval; 3D NGTDM's IBSIIis never cleared out of core;get_metaparam("3glcm/...")reads the 2D settings.
Please file an issue for each one. Then on the page, call it a defect and link the issue, and add a comment in the script naming the issue next to each assertion that pins one. That way a fix updates the page on purpose.
Nits are inline. That includes the "a fix is coming" promises, about 40 of them, gathered into one comment on installation.rst.
Comments on files GitHub shows without a diff
This PR is past GitHub's diff limit, so these files can't take line comments. Here they are with links to the head commit.
-
nyxus.py:64: coarse_gray_depth doc omits that ibsi=True disables texture binningBlocking: This says
coarse_gray_depthis the number of grey levels the texture features bin to. Withibsi=Truethat is not so: the texture families skip binning and read the raw levels (docs/source/cli_reference.rst:150in this same PR says so for--ibsi, andibsi_mode.rst:36too). I checked on this branch: 40x40 random uint16 ROI,Nyxus(["GLSZM_SAE","NGTDM_COARSENESS","GLDM_SDE","GLRLM_SRE_AVE","NGLDM_LDE"], coarse_gray_depth=d, ibsi=True)returns identical values for d=8 and d=64 (0.99718, 0.0015, 0.99438, 0.9993, 0.99438), while withibsi=Falsethey differ (0.70082 vs 0.95843, ...). OnlyENTROPY/UNIFORMITY/HISTOGRAMkeep following the depth. Please add this tocoarse_gray_depthand toibsi(which currently reads only 'compute the IBSI-compliant variant'), here and in the Nyxus3D/ImageQuality copies. -
nyxus.py:196: Note says set_params leaves computed features unchanged; HISTOGRAM changesBlocking: This note (repeated at
nyxus.py:1111,:1193,:1503,:1960,:2038,:2271,:2974) says changingcoarse_gray_depthafterwards 'updates what get_params reports but not the features computed by this instance'. On this branchn = Nyxus(["GLSZM_SAE","ENTROPY","HISTOGRAM"], coarse_gray_depth=64); n.set_params(coarse_gray_depth=8)leaves GLSZM_SAE and ENTROPY at their depth-64 values but the output now has 8HISTOGRAM_BIN_*columns instead of 64. So the feature set does change (its column count follows the new depth while the binning does not). Please state that, or the note misleads anyone who reads it as 'no effect'. This is the behaviour #486 replaces by refusing a changed depth, so the note needs rewriting after #486 in any case. -
nyxus.py:322: Code changed, contradicting the 'docstring-only' claimBlocking: The description says the docstring-stripped syntax trees of
nyxus.pyandfunctions.pyequal the previous ones. They don't: this line,nyxus.py:1582(Nyxus3D) andnyxus.py:2345(ImageQuality) change theValueErrortext from... must be non-negative.to... must be positive.(I diffed theast.unparseof both versions with docstrings removed; these three strings are the only difference). The new wording is more accurate, since 0 is refused, but it is a user-visible message change in a PR that states the code is untouched. Please either say so in the description or drop the three edits. Note that #486 removes these checks (it accepts 0 and negatives), sotests/python/test_api_arguments_mechanics.py, which assertsmatch="coarse_gray_depth.*must be positive", cannot survive whichever PR lands second. -
f_ngldm.rst:556-563: Cross-PR #486: NGLDM negative/zero-depth paragraph goes falseNit: This paragraph is right for
main, but #486 makes every sentence of it false. Whichever of #486 and this PR lands second has to rewrite it. In #486 NGLDM bins 0 and negative depths through the shared scheme (TextureFeature::ngldm_grey_levelintexture_feature.h). A negative depth becomes IBSI's fixed bin number over the ROI's [min, max], 0 keeps the raw levels, andNyxus(coarse_gray_depth=...)accepts both. I measured this page's worked example with a #486 build:depth GLMHGLCElevels −64 (page: 2.405e9, 6.847e18) 29.68 1241.2 1, 17, 33, 49, 64 0 (page: 0, 0) 2.8 9.28 1–5, the IBSI-mode values #486 makes three other statements on this page true only for a positive depth: the tip at lines 31–32 ("does not use the grey-level binning of the other texture families"), lines 49–50 ("its own rule, not with the binning that GLCM, GLRLM and GLSZM share"), and the Settings row at 164–166 ("A negative value does not select PyRadiomics-style binning").
ngldm_worked_example.pyasserts none of these values, because the constructor refuses them onmain. So nothing fails after the merge and the page goes stale silently. Once both PRs are in, the script can pin the −64 and 0 values from Python. -
f_ngldm_3d.rst:483-484: set_params ignores a positive coarse_gray_depth tooNit: Same point as on the 3D GLDZM page: "
set_paramsignores 0" undersells it. On main a positiveset_params(coarse_gray_depth=N)is accepted and shows up inget_params(), but the NGLDM grey levels keep the constructor's depth: per-family settings are compiled only in the constructor (src/nyx/python/new_bindings_py.cpp:127). Please say that only the constructor sets the depth, ascmdline_and_examples.rst:218does. -
f_ngtdm.rst:20-24: Name the out-of-core IBSI cause instead of "a fix is coming"Nit: The behaviour described here is right; I reproduced it with two ROIs out of core in IBSI mode. ROI 1 matched, and ROI 2's coarseness was 0.0405 instead of 0.0669. "(a fix is coming)" will go stale in user docs, though. The cause is that
NGTDMFeature::Iis a member thatclear_buffers()never clears (ngtdm.cpp:19-31), and the IBSI branch appends 0..max to it (ngtdm.cpp:56-61) on the feature instance that the out-of-core path reuses across ROIs. I'd link the tracking issue in place of the promise. It's IBSI-only: at--coarseGrayDepth=0the rows come fromlower_boundand still match, which I checked.f_glcm.rst:789(constant ROI returns 0 whatever--novalis) makes the same "a fix is coming" promise. There the cause is thatcalculate()writes the substitute intor.fvalsandsave_value()then overwrites it with the empty per-angle buffers (glcm.cpp:29-96,:143-208), and the writer pads those with 0.0 (output_2_buffer.cpp:216-217). Naming that is more useful than the promise. -
f_ngtdm_3d.rst:31-33: A lowest intensity of 0 also reproduces PyRadiomics busynessNit: "with any other lowest intensity, all features but busyness do" is wrong for a lowest intensity of 0. When the cube's lowest level is 0, every level is raised by one (
src/nyx/features/3d_ngtdm.cpp:222-228). A ROI of 0..k that fills its box therefore gets levels 1..k+1, which is PyRadiomics'binWidth=1numbering, and busyness matches too. The vetting phantom is this case: intensities 0..5, busyness vetted at rel 1e-9 (ngtdm3d.pyradiomics_binwidth1intests/vetting/config_recipes.md). On the worked example minus 1,3ngtdm/greydepth=0gives busyness 1.47697, the page's 1.477. I'd write "whose lowest intensity is 0 or 1 ...; with a higher lowest intensity, all features but busyness do". -
f_ngtdm_3d.rst:257-258: Complexity reads as divided twiceNit: The 2D definition this links to already divides by N_{v,p} (
f_ngtdm.rst:256), so "divided by the number of visited voxels" reads as a second division. The code divides once (3d_ngtdm.cpp:473,sum / Nvp), and the worked example's 5.241 is that single division. I'd write "with N_{v,p} the number of visited voxels", matching the Contrast entry below. -
f_ngtdm_3d.rst:327-329: Constant-ROI-without-corners values need the intensityNit: Contrast 31.52, complexity 18.57 and strength 1.176 depend on the block's intensity: the two levels are the background's 1 and the intensity plus one. The script uses 7 (
docs/examples/ngtdm_3d_worked_example.py:92), and the page doesn't name it, so a reader can't reproduce the numbers. Please write "a constant 3 × 3 × 3 block of intensity 7 without its eight corners". -
f_pixel_intensity.rst:879-881: coarseGrayDepth=0 is an out-of-bounds write; #486 changes all of thisNit: On main, "undefined" means memory-unsafe. With
--coarseGrayDepth=0,TrivialHistogram::initializesizesbins_cust_to 1 and then runsbins_cust_[n_custBins - 1] += ...with index −1 (src/nyx/features/histogram.h:82), a write before the buffer. The CLI accepts 0 silently (environment.cpp:669, no range check) and printsENTROPY=-0,UNIFORMITY=0. The CLI should refuse it, as Python does, rather than the docs describing a result.These sentences also clash with #486, which is open:
histogram_bin_count()there maps 0 to 24 bins and a negative N to |N| bins for HISTOGRAM and IH, and Python then accepts every 32-bit integer. If #486 lands first, this paragraph, line 175-177 ("HISTOGRAM writes no columns (a fix is coming)", "Python accepts only positive values"), f_intensity_histogram.rst:185-189 and 731-732 (0 and negatives give the noval substitute), and the HISTOGRAM text in featurelist.rst all go stale. Whichever PR merges second needs to update them. -
f_radial_distribution.rst:60-62: Whole-slide radial centre lands on the image's top edge, undocumentedNit: With the four-corner contour the coarse-to-fine search degenerates, and the page does not say so. For
n = 4,max_sqdist/min_sqdist(src/nyx/features/pixel.cpp:41-68,:114-141) start withstep = int(4/ln 4) = 2, so they compare corners 0 and 2 only; when corner 0 wins they stop on the window[0, 2)and never look at(W, 0)or(0, H). Every pixel on the perpendicular bisector of the(0,0)-(W,H)diagonal then has spread 0, andfind_center(pixel.cpp:146) takes the first one read, on the top rows near the right edge, not the middle of the image. I get the centre at(19, 1)on a 20 × 20 slide (R = 19.03, 11 pixels in ring 0) and at(50, 0)on a 64 × 48 slide (R = 50, 88 pixels in ring 0);featurize_files(..., single_roi=True)returns exactly the ring fractions those centres give.*WHOLESLIDE*includes these features, so please add this under Possible issues with the measured values (the rings are centred on an edge pixel, and the outer rings cover only part of the slide), and file it as a defect. -
f_radial_distribution.rst:449-450: "in memory" is ambiguous in the tie exampleNit: Both values here come from the in-memory path; the difference is array input against file input (
radial_worked_example.py:178-179). Elsewhere on the page "in-memory" means "not out-of-core", so I'd write "9 for the array and 2 for the same image read from a file". -
f_radial_distribution.rst:459: Pages disagree on the 15-ellipse out-of-core countNit: This page says 2 of 15 random ellipses at the origin differ out-of-core, but
large_images.rst:222-223says "one or two of 15". It is one measurement, so both pages should quote the same number. -
f_zernike.rst:408-414: Dropping the centre pixel is a Zernike bug, not a conventionNit:
zernike.cpp:273skipsr < DBL_EPSILONonly because ofa = x/rand theH3/r2term (zernike.cpp:282-283, 322). ButR_n0(0) = (-1)^{n/2}, so the pixel does contribute to everym = 0moment. Every symmetric ROI with an odd side loses its centre pixel, as the 3 × 3 example shows (Z00.2387 instead of 0.3183). The fix is small: atr = 0, addconst_t * (-1)^(n/2)toAR[n][0]and skip the recurrence.I'd file it and label this paragraph as a known defect.
zernike_worked_example.py:94-95pins the wrong value (z[0]*pi == 0.75) and will need to change with the fix. -
nyxus.py:492: file_pattern is a full match, not statedNit: 'Regular expression selecting the files, matched against file names' does not say the whole name must match.
file_pattern=r"c0"selects nothing here and raisesRuntimeError: ... no intensity and/or label files to process, whiler".*c0.*"works, and the case must match (.*_C0\.ome\.tiffinds nothing). Say 'full match, case-sensitive' (same wording at:1744,:2416). -
nyxus.py:902: Per-file blacklist matches the bare file name onlyNit: The per-file form
file1.ome.tif:5,6,7only works with the bare file name.blacklist_roi("seg/p0_y1_r1_c0.ome.tif:1,2,3")is accepted without error but removes nothing (139 rows before and after), whilep0_y1_r1_c0.ome.tif:1,2,3removes 3. Please say that a directory part is not matched, since the summary string echoes whatever was passed and gives no hint.
What #485 and #486 make false, page by page
Guide, docstrings, generated reference. All of these are #486, and true on main.
conventions.rst:- 152-155: only a positive depth is accepted.
- 158-166: NGLDM, and
HISTOGRAMat a negative depth. - 177-178:
glcm/greydepth. - 182-203: the 3D families "0: no binning;
--coarseGrayDepthhas no effect"; GLDZM at a negative depth. - 233-238: the 3D default binning.
- 333-346: the 3D background rows.
cli_options.toml:277-278and the generatedcli_reference.rst:150: "3D GLRLM, GLSZM, GLDM and NGTDM ignore it", the NGLDM rule,HISTOGRAMwriting no columns when negative, "8.5 runs as 8" (#486 refuses1.5).ibsi_mode.rst:68-69;featurelist.rst:148, 2500, 2613, 2678, 2882.nyxus.py:- 64, 1400 and 2214: "must be positive"; 3D GLRLM/GLSZM/GLDM/NGTDM "ignore it ... 0 by default";
ENTROPY/UNIFORMITY/HISTOGRAMusingcoarse_gray_depthbins (#486:abs(depth), 24 at 0). - 405-406:
glcm/greydepth. - The eight
set_paramsNotes: #486 raises for a different depth and drops it fromset_environment_params.
- 64, 1400 and 2214: "must be positive"; 3D GLRLM/GLSZM/GLDM/NGTDM "ignore it ... 0 by default";
test_api_arguments_mechanics.pyasserts "must be positive"; #486 removes that check.directory_and_params.py:28,34andcmdline_and_examples.rst:214-236:set_params(coarse_gray_depth=256).
2D feature pages.
- #485:
f_glrlm.rst265-267, 284-287, 321-323, 341-342, 459-460, 478-481 and 507-531;glrlm_worked_example.py:79-87;f_glszm.rst284-285, 359-360 and 532-544;glszm_worked_example.py:102-116. - #486:
f_ngldm.rst31-32, 49-50, 164-166 and 556-563;f_gldzm.rst170-173 and 562-570;f_glcm.rst187-188; "(command line only)" on six pages;f_pixel_intensity.rst175-177 and 879-881;f_intensity_histogram.rst185-189 and 731-732. - Generated indexes: with #485, the 2D and 3D GLRLM and GLSZM indexes and
featurelist.rst; with #486, the 2D and 3D GLDZM and NGLDM indexes.
3D feature pages. I measured these values on a Release build of #486's head, on the pages' worked examples; #485's values are closed form. On the 3D pages #485 breaks no script, because none of the values it changes is asserted, but the text goes stale.
#486, default binning of 3D GLRLM / GLSZM / GLDM / NGTDM moves from unbinned to 64 MATLAB levels
f_glrlm_3d.rst:
- 34-36, 97-99, 214-218 and 225-228: "no binning by default", "
--coarseGrayDepthdoes not apply".coarse_gray_depth=2now gives the3glrlm/greydepth=2values. - 145-146: "default settings and IBSI mode give the same values". The default differs on GLV and the six grey-weighted emphases: GLV 229.38, HGLRE 1706.6 vs 6.435. The index values are IBSI-mode values; say so in the caption.
- 102-104 and 177-178: background voxels form level-1 runs, and N_g counts the bounding box's levels. Background is now level 0 from the voxel cloud, and N_g counts the ROI's levels.
- 399-401 and 460-474 (RP above 1):
- two-corner mask at
greydepth=2: RP 0.72, LGLRE 0.4583 (was 0.8 / 0.5125); - ellipsoid: RP 0.9396 / 0.9850 / 0.9926 at 16 / 64 / 128, RP_AVE 0.9854 at 64 (was 1.131 / 1.182 / 1.191 and 1.130);
- "without binning" now needs
greydepth=0.
- two-corner mask at
- 476-479: "the intensity by default". It is now the bin number by default.
- 499-500: "Without binning, the default".
- 515-516: #486 adds
TEST_3D_GLRLM_BOUNDED_FEATURES_INVARIANT. glrlm_3d_worked_example.py53-57, 64-69, 79-83 and 84-86 fail.
f_glszm_3d.rst:
- 31-33, 49, 178-184 and 195: "0 by default", "
--coarseGrayDepthhas no effect".coarse_gray_depth=8gives GLV 4.617, HGLZE 38.0. - 61-62, 141-145 and 399-401: "background is 1 with MATLAB-style binning", "levels present in the bounding box". Background is level 0, and level-1 voxels form zones.
- 92-93: the default matrix has rows 17, 33, 49 and 64, not 1 to 4. The default GLV is 389.36 vs 1.580 in IBSI mode.
- 420-428 ("drops the lowest level"): fixed.
greydepth=2gives 4 zones, ZP 0.1481, SAE 0.5283, LAE 123.75, LGLZE 0.8125. - 441-445 ("ignored in whole-volume mode"): whole-volume now reads
3glszm/greydepth, ZP 0.1481. - 482: MATLAB-style binning is now the default.
glszm_3d_worked_example.py83-85, 97-101 and 121-128 fail.
f_gldm_3d.rst:
- 32-35, 51, 182 and 190: "0 by default", "
--coarseGrayDepthhas no effect".coarse_gray_depth=8gives GLV 3.757, HGLE 36.10. - 63 and 139: "background is level 1 when N > 0", "levels in the bounding box".
- 93: the default rows are 17, 33, 49 and 64.
- 367-374 (dimmest voxels dropped): fixed.
greydepth=2: Nz 48, SDE 0.02213, LDE 116.08, GLN 29.04, GLV 0.1975.greydepth=1: computed, SDE 0.006641, LDE 239.4.
- 376-381: default HGLE 1916.8 on both volumes.
- 384: an intensity-0 ROI voxel is counted under the default (SDE 0.08743, LDE 32.96).
- 414: "the default (no binning)".
gldm_3d_worked_example.py66-69, 73, 82-83, 85 (get_metaparamnow returns 64), 89 and 123 fail.
f_ngtdm_3d.rst. #486 changes the default but not NGTDM's background handling.
- 30-33, 46, 63, 82, 105-106, 172, 185-186 and 205: the default is 64 levels, and
--coarseGrayDepthapplies.coarse_gray_depth=8gives coarseness 0.08807, busyness 1.518. - Default values on the worked example: coarseness 0.009831, contrast 734.1, busyness 1.485, complexity 1294.3, strength 11.13.
- They change
ngtdm_3d_index.inc17-37 and[ngtdm_3d]infeature_extras.toml628-632, unless the page moves its examples to3ngtdm/greydepth=0.
- They change
- 228-231: default busyness 1.4846; the shift by 9 gives 0.9094.
- 294-305: under the default, voxels outside the mask and intensity-0 voxels are level 1 and are not visited. Corners off gives 0.01215 / 551.8 / 1.062 / 1486.7 / 15.39.
- 317 (radius 2): 0.01005 / 722.8 / 1.453 / 1276.8 / 11.30.
- 327-329 (constant ROI, corners off): coarseness 0.00407, the other four 0.
- 340: the default rows are the bins present.
- 353: the default output is now unvetted; the vetted rows are at
greydepth=0. ngtdm_3d_worked_example.py36, 61, 64-67, 73-75, 81 and 97 fail (default-settings checks; the IBSI checks pass).
#486, signs of the coarse grey depth, and set_params
f_intensity_3d.rst49, 162 and 185: depth 0 gives 24 histogram bins (main writes before the buffer). Python accepts any 32-bit integer.f_glcm_3d.rst187-188 and 192:Nyxus3Dno longer rejects a depth below 1.f_gldzm_3d.rst:- 194: "Python accepts only a positive value". Depth 0 gives GLM 3.286, and -8 gives 5.286.
- 472-474:
set_paramswith a depth other than the current one now raisesValueError. - 485-493: #486 adds a third, re-binning MIRP configuration (
gldzm3d.mirp_fbn20_negdepth, 16 vetted rows).
f_ngldm_3d.rst:- 172: a negative depth now selects PyRadiomics-style bins. At -20: GLM 9.889, HGLCE 147.2.
- 467: the other four 3D families now bin at the coarse depth.
- 480-484: 0 gives the raw levels (GLM 2.407, the IBSI value), not every voxel at level 0. A negative depth no longer wraps to 2³²−|N|.
- 504: a third MIRP setting,
ngldm3d.mirp_fbn20_negdepth.
f_glrlm_3d.rst:- 264-266: "with the differences in integer arithmetic noted below".
- 305-306: "Unlike the 2D code". 2D HGLRE is squared in double too.
- 338-340: LRHGLE becomes 1.2308 × 10⁹ instead of 1.0529 × 10⁹.
- 443-445: SRLGLE becomes 3.374 × 10⁻⁹ instead of 0.
- 481-500: the overflow section and its table go entirely.
- The index Validation cells of the eight emphases gain the analytic rows.
f_glszm_3d.rst456-465: LAE becomes 2 047 936 001 and SAE 0.5. The Validation section should nameTEST_3D_GLSZM_LARGE_ZONE_SIZE_EMPHASIS_ANALYTIC, and the index cells of SAE and LAE gainanalytic (rel. 1e-12).
Unaffected by both:
- every IBSI-mode example value in
feature_extras.toml; - the 3D shape page;
- the 3D GLCM values;
- GLRLM
greydepth=2without a mask hole; - GLSZM
greydepth=-3.
| group = "Texture settings" | ||
| required = false | ||
| default = "64" | ||
| default_at = "environment.cpp:106" |
There was a problem hiding this comment.
Blocking: default_at (here environment.cpp:106, and the same for --dim, --resultFname, --ibsi, --pixelDistance, --reduceThreads, ...) names one source line, and gen_cli_reference.py --check (also run by test_docs_cli_reference_table_matches_code_mechanics on every pytest tests/python run) fails when that line does not assign the default. Inserting an include or a line above line 124 in environment.cpp is enough. #486 adds three #include lines at the top of environment.cpp and rewrites the --coarseGrayDepth parsing; I ran this branch's docs/ against #486's src/ and --check reports --dim, --resultFname, --ibsi, --pixelDistance, --coarseGrayDepth and --reduceThreads as 'not the value assigned'. So every PR touching environment.cpp, environment_basic.h or dirs_and_files.cpp fails the Python suite until someone renumbers this table by hand, which punishes unrelated code changes. Locate the assignment by symbol instead (a member name, e.g. default_at = "environment.cpp:n_pixel_distance" matched by regex, or read the value from the member's initialiser) so a code move does not break the check, while a changed default still does.
There was a problem hiding this comment.
Fixed default_at now names the variable or call that is assigned the default (environment.cpp:n_pixel_distance), and --check requires exactly one literal assignment of it in that file. New tests add lines above every default (still passes), change a default in a copy of the source (caught) and add a second assignment (refused). Done in 8dfe1b0.
|
|
||
|
|
||
| @pytest.mark.parametrize("script", _params()) | ||
| def test_2d_docs_examples_run_mechanics(script, tmp_path, extras_json): |
There was a problem hiding this comment.
Blocking: test_2d_docs_examples_run_mechanics runs every script in docs/examples/, including shape_3d_worked_example.py, glcm_3d_worked_example.py, volume_files.py, volume_channels.py and so on, and test_2d_readme_quick_starts_run_mechanics runs the README blocks whatever they use. SPEC 6.1 makes the dim token name the implementation under test and requires a function's token to match its file's. Either split into test_2d_docs_examples_mechanics.py and test_3d_docs_examples_mechanics.py (selecting scripts by name), or follow what you did for test_docstring_examples_mechanics.py and list this file in DIM_AGNOSTIC in tests/vetting/check_test_names.py with a reason and drop the 2d token. As it stands check_test_names.py passes only because the checker cannot see which scripts the parametrisation selects.
There was a problem hiding this comment.
Fixed Took the second option: the file is now test_docs_examples_mechanics.py, its functions dropped the 2d token, and it is listed in DIM_AGNOSTIC with the reason that it runs 2D, 3D and dimension-free scripts alike. Every reference to the old name in the scripts and the developer guide is updated. Done in 8dfe1b0.
| run: python -m sphinx -b html -W --keep-going docs/source docs/build/html | ||
|
|
||
| - name: Check links | ||
| run: python -m sphinx -b linkcheck -W --keep-going docs/source docs/build/linkcheck |
There was a problem hiding this comment.
Nit: linkcheck -W fails the job on any transient 403/429/timeout. The pages link to ncbi.nlm.nih.gov, imagej.net, github.com/PolusAI (many anchors) and others, all of which rate-limit or time out from GitHub runners now and then, so an unrelated PR gets a red check. Make the link check a separate non-required job (or continue-on-error, or a weekly schedule: trigger), and set linkcheck_retries/linkcheck_timeout and linkcheck_ignore for the rate-limited hosts in conf.py, which sets none of them. Also add permissions: contents: read (the other release workflows set permissions; this one gets the default token scope) and pip caching (cache: pip on setup-python).
There was a problem hiding this comment.
Fixed The link check is now its own Check_Links job with continue-on-error: true, so a rate-limited host no longer fails the workflow, and conf.py sets linkcheck_retries, linkcheck_timeout and linkcheck_rate_limit_timeout instead of an ignore list. The workflow also gets permissions: contents: read and pip caching. Done in 8dfe1b0.
| - 'src/nyx/featureset.cpp' | ||
| - 'src/nyx/env_features.cpp' | ||
| - 'src/nyx/features/*.h' | ||
| - 'tests/vetting/oracle_coverage.csv' |
There was a problem hiding this comment.
Nit: gen_cli_reference.py --check reads environment.cpp, environment_basic.h, dirs_and_files.cpp, main_nyxus.cpp, features/*.cpp etc. (every file named in an evidence/default_at), but the filter lists only cli_option_constants.h, featureset.*, env_features.cpp and features/*.h. A PR that changes a default in environment.cpp does not run this workflow; the drift is then caught only by the pytest wrapper, and the docs build and the staleness check of cli_reference.rst never run for it. Add src/nyx/*.cpp/*.h (or src/nyx/**) to both filters.
There was a problem hiding this comment.
Fixed Both path filters now use src/nyx/** in place of the hand-picked files, so a PR that changes any cited source file runs the docs workflow. Done in 8dfe1b0.
| @@ -17,17 +16,11 @@ dependencies: | |||
| - numpy | |||
| - pip>=20.1 | |||
| - pyarrow | |||
| - fmjpeg2koj | |||
| - fmjpeg2koj | |||
| - arrow-cpp[version='>=10.0.0'] | |||
There was a problem hiding this comment.
Nit: This drops the https://repo.continuum.io/pkgs/free channel and parquet-cpp from the build environment in addition to the sphinx entries. Docs-tool removal is fine (they move to docs/requirements.txt), but parquet-cpp is a build dependency of the Arrow/Parquet path and its removal is a build-environment change that the description does not mention. Keep that out of this PR or say why it is safe (conda-forge libparquet/arrow-cpp provide it).
There was a problem hiding this comment.
Fixed Restored the repo.continuum.io/pkgs/free channel and parquet-cpp; only the sphinx entries leave environment.yml, so the build environment is unchanged. Done in 8dfe1b0.
| Different default pairs on the command line and in Python | ||
| Without ``--gaborfreqs`` and ``--gabortheta``, the command line pairs frequencies | ||
| :math:`0, \pi/4, \pi/2, 3\pi/4` with orientations of 4, 16, 32 and 64 radians: the built-in list | ||
| stores each frequency where an orientation belongs, and the reverse. Python uses frequencies 4, | ||
| 16, 32, 64 at 0°, 45°, 90°, 135°. The same image gives different ``GABOR`` columns from the two | ||
| unless both lists are given. |
There was a problem hiding this comment.
Nit: Three items under Possible issues are bugs, not conventions:
- The built-in pairs have frequency and angle swapped (
gabor.cpp:19-25). - The CPU path reads at
ceil(n/2)(gabor.cpp:493,496) while the GPU reads atfloor(n/2)(gpu/gabor.cukerCalcEnergy,halfpad = kerside / 2). So for odd sizes the two paths count different pixels. - The GPU path has no constant-baseline guard (
gabor.cpp:274-306, compared with89-95).
None of them has an upstream issue. I'd file each one and say on the page that it is a known defect, with the link. Otherwise the fix will look like a behaviour change. The docs test pins one of them: gabor_worked_example.py:67-68 asserts the shifted kernel-size-15 values, so it will fail when the offset is fixed.
|
|
||
| # Z0 is 1/pi when every pixel lies in the disk; Z1 is 0 because the disk sits on the centroid | ||
| assert math.isclose(features["ZERNIKE2D_Z0"][0], 1 / math.pi, rel_tol=1e-12) | ||
| assert abs(features["ZERNIKE2D_Z1"][0]) < 1e-16 |
There was a problem hiding this comment.
Nit: ZERNIKE2D_Z1 is pure rounding residue. Its size depends on summation order and FMA contraction, which differ between compilers and architectures. Here (arm64) it is 2.0e-17 on the example and 4.0e-17 on its rot90, so the < 1e-16 bound leaves a margin of 2.5-5×. The analytic test uses a 1e-15 floor for the same column (tests/test_2d_zernike_analytic.h:58). I'd use 1e-15 here, and on the page (f_zernike.rst:89-90) say "a rounding residue of order 10^-17" rather than quote a bound that only one platform shows.
There was a problem hiding this comment.
Fixed The bound is now 1e-15, and the page says "a rounding residue of order 10^-17". Done in 26d3205.
| at output time (by ``--noval`` in CSV and pandas, by ``-0.0`` in Arrow and Parquet). ROIs | ||
| whose padded image times the number of filters exceeds cuFFT's limit of :math:`2^{27}` values | ||
| are refused. The GPU values are not checked in continuous integration. |
There was a problem hiding this comment.
Nit: conv_dud_gpu_fft_multi_filter refuses when 2 * size * n_filters >= 2^27 (src/nyx/gpu/gabor.cu:343-344). The 2 counts real and imaginary doubles, and n_filters is pairs + 1 because the baseline is one of the filters (gabor.cpp:255-269). A ROI is therefore refused at (H+n-1)(W+n-1) * (pairs+1) >= 2^26 complex values, not at 2^27. Also, on the command line "refused" means GaborEnergyGPUMultiFilter returns early with all-zero responses, so every column becomes 0/0 and is written as the --noval value. Only the Python build throws (gabor.cu:347-349).
There was a problem hiding this comment.
Fixed The entry now gives the refusal at (H+n−1)(W+n−1) × (pairs + 1) reaching 2^26 complex values, half of cuFFT's 2^27 doubles, and says a refused ROI gets all-zero responses, written as that 0/0 result. Done in 26d3205.
| # The baseline wavelength is 62.8 pixels; settings are read in single precision | ||
| assert round(2 * math.pi / 0.1, 1) == 62.8 | ||
| assert f"{float(np.float32(0.1)):.9f}" == "0.100000001" |
There was a problem hiding this comment.
Nit: f"{float(np.float32(0.1)):.9f}" == "0.100000001" passes whatever Nyxus does with the setting, so it doesn't back the page's claim that every Gabor value is read in single precision (f_gabor.rst:282-283). Nyxus reports the value it stored. nyx.get_params("gabor_gamma")["gabor_gamma"] == float(np.float32(0.1)) holds today (it returns 0.10000000149011612), and so does the same check on gabor_freqs. Asserting that would test the claim.
There was a problem hiding this comment.
Fixed The script now reads gabor_gamma and gabor_freqs back with get_params and asserts that they equal their single-precision values. Done in 26d3205.
| # A ROI of one intensity reports 0, whatever its shape | ||
| for m in (mask, disk(6), np.ones((7, 9))): | ||
| assert erosions(m, values=np.full(np.pad(m, 3).shape, 5, dtype=np.uint32)) == 0, "constant ROI" |
There was a problem hiding this comment.
Nit: EROSIONS_2_VANISH reads only the mask (erosion.cpp:36 sets every ROI pixel to 1), yet ErosionPixelsFeature::parallel_process_1_batch skips a ROI with aux_min == aux_max (erosion.cpp:158), and osized_calculate copies the skip (erosion.cpp:120). So featurize(mask, mask), or any binary or label image passed as the intensity, reports 0 for every ROI, while the same shape with textured pixels reports 6 (the disk of radius 6). The skip is a leftover of the intensity families' degenerate-ROI guard, not part of the definition, and whole-slide mode already does not apply it (page lines 292-297), so the same shape gives different values in the two modes. The page describes it accurately (lines 61-62, 204-205, 285-290), but these three assertions make the fix (drop both checks) fail the docs suite. I'd file it, link it from the Possible issues entry, and say in the script that these lines pin a known defect. conventions.rst's constant-ROI table (Erosion row) needs the same link.
There was a problem hiding this comment.
…g CI
gen_cli_reference.py --check located each documented default by a line
number, so adding a line above it in any of about 25 source files failed
the Python suite. Each default_at now names the variable or call that is
assigned the default ("environment.cpp:n_pixel_distance"), and the check
requires exactly one literal assignment of it in that file. Moving code
no longer fails the check; changing a default or assigning it twice does.
Tests copy src/nyx, insert lines, change a default and add a second
assignment.
The docs workflow runs for every change under src/nyx, which the
generators read, takes read-only permissions and caches pip. The link
check is its own job with continue-on-error, and conf.py retries links
with a longer timeout. docs/requirements.txt bounds every package.
The docs example test is test_docs_examples_mechanics.py, listed in
DIM_AGNOSTIC: it runs 2D, 3D and dimension-free scripts alike.
environment.yml keeps parquet-cpp and its channel list; only the Sphinx
packages leave it, for docs/requirements.txt.
The --gaborkersize entry says 1 reads past the convolution buffer.
Registry: - EROSIONS_2_VANISH is a regression pin. scikit-image's count agrees on the test shape with another structuring element and counting rule, so it confirms neither; the assertion moves to the regression file. - The six 2D GLRLM grey-level emphases name PyRadiomics, the source of their 1e-9 tolerance; IBSI's consensus values agree only to 1e-2. Worked-example scripts: - fractal: drop the 4294967295 in-memory pin, an out-of-range double-to-uint32 conversion whose result the compiler decides; assert 0 for a 2 x 2 square without its sign; add the anisotropic disk in memory and out of core, and a 256 x 256 whole-slide image (1.8667). - ngldm, glszm_3d, glrlm_3d: assert the quoted values they skipped; the headers name what stays unasserted (32-bit overflow results, command-line-only depths). - gabor: assert the stored single-precision settings; zernike: bound the Z1 rounding residue at 1e-15. Pages: - GLCM: positional features under PyRadiomics binning (DIFENTRO, SUMENTROPY; IDN/IDMN divide by the levels present), DIFAVE, SUMAVERAGE and IV as a known defect, and the diagonal labels. - Morphology: the holed square's inner loop is 13.66; AREA_UM2 is 0 because the resolution gate is never set, and its MATLAB check sets it itself. - Fractal: the whole-slide box is one pixel larger than the image. - Image quality: POWER_SPECTRUM_SLOPE reads past its radius table. - Gabor: kernel size 1 reads past the buffer; the GPU size limit. - 2D GLDM, GLDZM, GLSZM, NGLDM, NGTDM, GLRLM: comparisons with IBSI consensus values are consistency checks, not registry validations. - 3D GLDZM: merging PyRadiomics binning is not tested. - Wording and numbers on the GLSZM, GLDZM, 3D intensity, 3D GLCM, 3D NGTDM, radial, moments, histogram, Zernike and erosion pages, the feature-page template sentence, and the README and landing page. - Supported formats and the OME-Zarr fixture README: bioformats2raw 0.9 writes big-endian arrays, 0.12 little-endian (both measured). Docstrings: ibsi=True skips texture binning; file_pattern must match the whole name; a per-file blacklist names the mask file alone.
Every defect the pages describe now says so and links its issue (PolusAI#489-PolusAI#528), and every "a fix is coming" with an issue behind it is replaced by the link; packaging gaps are called known issues. The worked-example scripts name the issue next to each assertion that pins a defect's current value. New Possible issues entries, with measured values: 3D shape under single_roi=True (the merged mask's bounding box), morphology codes that need companion families, the whole-slide extrema, the whole-slide radial centre, and the 3D GLCM metaparameter getter. The "is coming" lines left describe behaviour that PolusAI#486 changes.
|
|
vjaganat90
left a comment
There was a problem hiding this comment.
I re-checked 8dfe1b0, 26d3205 and ff8f931 against every thread and against the points in my review body. I also ran the docs suite and the 2D/3D worked examples on a build of this head. Most of it is done:
default_atnow names the assignment.gen_cli_reference.py --checkpasses with #486'senvironment.cppand still fails on a changed default.- The description now matches the branch.
EROSIONS_2_VANISHis a regression row.- The examples test is
DIM_AGNOSTIC. - #489–#528 exist and match the defects that cite them.
- The fractal UB pin is gone.
I'm fine with deferring the #485/#486 items to the rebase, as long as this lands after both. That includes three things that are wrong today:
glszm_worked_example.py:102-116still asserts a signed-overflow result. On macOS arm64 the 300 × 300 case gives 1902426353, not the page's -245057295.f_glszm_3d.rst:458-465quotes an LAE wrap that this build doesn't produce. It returns the true 2047936001.- The
f_glrlm.rstoverflow section still leaves outLRE.
So this isn't an LGTM yet. I'll re-review after the rebase.
Blocking
f_glcm.rst:801-809:JVARandDIFVARare still listed as position-based. Both subtract a mean computed from values (thread reply).
Nits
- Thread replies:
- Inline:
- three fractal assertions pin defects without naming them;
- the morphology and radial headers claim values they don't assert.
cli_options.tomlevidence: the ranges are still line numbers, and--checkonly checks that a range exists, so they go stale silently. With #486, every range pastenvironment.cpp:665is 6 lines early. Please re-cite them at the rebase.
The CLAUDE.md/CONTRIBUTING.md change now has its own heading as a policy change. I've left that thread open for the maintainers.
| for row in from_files(labels, (labels > 0) * 100): | ||
| check(row[:2], [1.8416, 1.0], "two labels") | ||
| check(from_files(labels, (labels > 0) * 100, mergerois=True)[0][:2], [1.4231, 1.107], "merged") | ||
| check(from_files(np.ones((200, 200)), single_roi=True)[0][:2], [1.8703, 1.0], "whole slide") |
There was a problem hiding this comment.
Nit: The description says each assertion that pins a defect names its issue, and the 256 case below does, but three here don't. Line 161 is the 200 × 200 whole-slide value 1.8703 (#493; it equals the mask-of-ones value only because 201 and 200 both round up to a 256 canvas). Lines 145 and 155 are the out-of-core two squares 1.0 and disk 1.0482 (#509). Please add the # pins known defect comment above each.
| @@ -0,0 +1,159 @@ | |||
| # Example for docs/source/Math/f_morphology.rst: 2D morphology features of the page's worked | |||
| # example, a notched ROI (label 1) with a small rectangle touching it (label 2). The page shows the | |||
| # lines between the "example" markers. The assertions below pin every value the page quotes, so the | |||
There was a problem hiding this comment.
Nit: This header says every value the page quotes is pinned, but two new entries aren't. The "Codes that need companions" entry in f_morphology.rst quotes MAJOR_AXIS_LENGTH 39.36, ORIENTATION 45.93, ROUNDNESS 0, DIAMETER_CIRCUMSCRIBING_CIRCLE 35.61 and HEXAGONALITY 0 when requested alone, and the whole-slide entry quotes the 64 × 48 extrema. I reproduced all of them, so they're right today, but no assertion would catch a change. The same goes for radial_worked_example.py: the new #516 entry's centres (19, 1) and (50, 0) and the ring-0 counts 11 and 88 aren't asserted. Please assert them, with the issue comments.
There was a problem hiding this comment.
Fixed The morphology script now asserts the companion-less ellipse, circle and hexagonality values (#495) and the 64 × 48 whole-slide PERIMETER and extrema (#493). The radial script checks the whole-slide centres (19, 1) and (50, 0) by matching Nyxus's ring counts, ring 0 being 11 and 88 (#516). Done in c6f8475.
Pages: - GLCM (2D and 3D): JVAR and DIFVAR join DIFAVE, SUMAVERAGE and IV under PolusAI#510. JVAR centres positions on JAVE, a mean of values, and DIFVAR centres position differences on DIFAVE, so neither is position-based when the levels present are not consecutive. - GLCM directions: name the neighbour of every angle. 90 degrees pairs a pixel with the one below, so it is transposed against MATLAB, PyRadiomics and IBSI as the diagonals are; only 0 degrees matches. - NGLDM: the two grey-level ladders are stated as what the formulas give, not as Nyxus output. Registry: the six 2D GLRLM grey-level emphasis _AVE rows name PyRadiomics, as their base rows do; featurelist.rst and the report are regenerated. Worked-example scripts: - shape_3d: assert the radius-5 ball as a segmented ROI and under single_roi=True (PolusAI#494); the 3 x 2 x 2 block fills its box, so it no longer carries the issue comment. - morphology: assert the codes requested without their companion family (PolusAI#495) and the 64 x 48 whole-slide perimeter and extrema (PolusAI#493); feature_extras.toml names the issue above each ROI 1 value that pins PolusAI#497, PolusAI#498 or PolusAI#500. - radial: assert the whole-slide centres (19, 1) and (50, 0) through the ring counts Nyxus reports (PolusAI#516). - fractal: name PolusAI#509 and PolusAI#493 above the three assertions that pin them. - glszm_3d: the header names the level-1 values as unasserted. - ngldm: drop the two ladder assertions, which never called Nyxus.
|
|
vjaganat90
left a comment
There was a problem hiding this comment.
I re-checked c6f8475 against my last round, and everything I raised there is done. All nine threads are settled:
JVARandDIFVAR, the blocking item, are now in the #510 sentence on both GLCM pages, each with the mean it subtracts. The 3D code mixes them the same way as the 2D code (3d_glcm.cpp:642-649,1066-1077).- The 90° neighbour is fixed.
- The six GLRLM
_AVEregistry rows are fixed, and the report is regenerated. - The NGLDM ladders are fixed.
- The morphology loop's #497, #498 and #500 marks are in.
- The 3D ball is asserted under
#494. - The 3D GLSZM header is fixed.
- The fractal issue comments are in.
- The morphology and radial assertions are in.
I built this head and ran the docs examples, the full pytest suite and the four --check generators. All pass, apart from the Arrow tests (my build has no Arrow), and no tracked file was rewritten. One nit inline.
This still isn't an LGTM, only because of the rebase. #485 and #486 haven't merged, so four items wait on it:
- the three values that are wrong today: the GLSZM overflow example, the 3D LAE wrap, and
LREin the GLRLM overflow section; - the
cli_options.tomlevidence ranges.
I'll approve once this is rebased onto both.
| r = Nyxus3D(["*3D_ALL_MORPHOLOGY*"]).featurize_files(["./ball5.ome.tif"], ["./ball5_mask.ome.tif"], | ||
| single) | ||
| for code, value in zip(codes, expected): | ||
| assert math.isclose(r[code][0], value, rel_tol=5e-4, abs_tol=1e-12), f"single_roi={single} {code} = {r[code][0]}, page says {value}" |
There was a problem hiding this comment.
Nit: The assert message sits on the same line after a run of spaces (152 columns). It looks like a lost line break. Please move the f-string onto its own line, as the assert at line 45 does:
assert math.isclose(r[code][0], value, rel_tol=5e-4, abs_tol=1e-12), \
f"single_roi={single} {code} = {r[code][0]}, page says {value}"The message of the single_roi ball assertion in shape_3d_worked_example.py sat after a run of spaces on the assert's line. It now follows a line continuation, as the other assertions in the script do. The parsed code is unchanged.
Summary
This branch brings the Nyxus documentation back in line with the code. The site is reorganized into a user guide, a feature reference, an API reference and a developer guide. Every feature family has a page in one template, each page's numbers are checked by a script that the test suite runs, and the feature list and option reference are generated from the code. The README shrinks to a landing page that links into the site.
Sixteen commits, one per area (build, entry pages, command line, input and output, scale and GPU, guide pages, feature catalogue, example tests, six groups of feature pages, API docstrings, developer guide), so the series can be reviewed commit by commit. Two commits then fix overflow examples on arm64/macOS, and three address the first review: the checks and CI, the pages, scripts and registry rows, and the issue links. Only the last commit is meant to build cleanly on its own.
Changes
Build and structure
docs/tools/gen_feature_reference.py,docs/tools/gen_cli_reference.py, each with a--checkmode). The feature list shows each feature's validation status fromtests/vetting/oracle_coverage.csv. The CLI reference finds each default by the variable or call it is assigned to (default_at = "environment.cpp:n_pixel_distance"), so moving code within a file does not fail the check and changing a default does.Feature pages (
docs/source/Math/), one template for every family: how the values are computed, a worked example, notation, settings with their real effect, output columns, a generated index with IBSI identifiers and validation status, one entry per feature with the formula as the code computes it, possible issues with measured values, validation and references.EROSIONS_2_VANISHis a regression pin (scikit-image's agreement on the test shape confirms neither the structuring element nor the counting convention), so its assertion moves fromtest_2d_morphology_skimage.htotest_2d_morphology_regression.h; and the six 2D GLRLM grey-level emphases name PyRadiomics, which their 10-9 tolerance comes from, rather than IBSI.User guide
Tested examples
docs/examples/, included withliteralinclude.tests/python/test_docs_examples_mechanics.pyruns each script in a fresh directory.docs/feature_extras.toml, which also feeds the index tables. A script's header names the values it does not assert: results of a 32-bit overflow on main, and values only the command line can produce (a negative or zero grey depth).API reference
nyxusmodule's docstrings are rewritten in numpydoc style and corrected against the code: defaults, group names, case-sensitivity, parameters that do not exist, and return values.ValueErrorthatNyxus,Nyxus3DandImageQualityraise for acoarse_gray_depthbelow 1 now says "must be positive" instead of "must be non-negative", since 0 is refused too.test_api_arguments_mechanics.pyasserts the new text. Everything else innyxus.pyandfunctions.pyis unchanged: their docstring-stripped syntax trees differ from the previous ones only in those three strings.Developer and project files
CONTRIBUTING.mdandCITATION.cff. release-please now updates the version inCITATION.cff.build-cliand an explicit Release build.plugin.json(the shipped WIPP manifest) offers thearrowipcoutput type that the program accepts, in place ofarrow; marksintDirandsegDiras required, since every run needs both; and drops about 50 feature options that the program does not accept (*ALL_BUT_GABOR*and*ALL_BUT_GLCM*, which define no features, and names such asEQUIVALENT_DIAMETERandTEXTURE_*that are not feature codes).tests/dataare corrected.Policy change in the contributor notes
CLAUDE.mdandCONTRIBUTING.mdchange one rule rather than correct it: a defect fix no longer gets a source comment describing the defect and the fix. Comments describe what the code does now, and the history of a change (the defect and the fix) goes in the commit message and the PR description. Maintainers may prefer to decide this separately; the two paragraphs can be dropped from this PR without affecting anything else.README
Test plan
python -m sphinx -b html -W --keep-going docs/source docs/build/html: passes, 0 warnings.python -m sphinx -b linkcheck docs/source docs/build/linkcheck: passes.python docs/tools/gen_feature_reference.py --check(now also checks the feature counts in README.md and index.rst) andpython docs/tools/gen_cli_reference.py --check: pass.pytest tests/python/test_docs_examples_mechanics.py: 48 passed (every example and worked-example script, and the README quick starts).pytest tests/python: 275 passed, 4 skipped on Python 3.12, and 259 passed, 5 skipped on Python 3.10 (the docs examples avoidtomllib, so they run on 3.10 too), including the docstring examples and the docs generator and configuration checks.tests/vetting/check_coverage.py --check,tests/vetting/check_test_names.pyandtests/python/test_vetting_mechanics.py: pass.nyxus.pyandfunctions.pydiffer from those before the change only in the threeValueErrorstrings named above.