Repository navigation
BUG: Fix remaining input checks in 6 filters and correct 3 docs - #1767
Open
imikejackson wants to merge 21 commits into
Open
imikejackson wants to merge 21 commits into
imikejackson wants to merge 21 commits into
Conversation
* attemptRename copied newPath.m_Path[i] for each index of oldPath. When the new prefix was shorter it read past its end, and when it was longer it dropped the extra components, so it was correct only for prefixes of the same depth. It now delegates to rebase() * Add DataPath::rebase(oldPrefix, newPrefix), which returns the path with the leading oldPrefix replaced by newPrefix, comparing whole components (a child "Edge Geometry Ids" does not start with "Edge Geometry"). It returns std::nullopt when the path does not start with oldPrefix * Add tests for rebase and for attemptRename with prefixes of different depth Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When a filter copied the other children of a geometry (or of a copied DataGroup) to a new location, it built each copied DataPath by replacing every occurrence of the source name in the whole path string. A child whose name contains the source name was renamed (Geom Feature Data under Geom became Geom Out Feature Data), and a parent DataGroup whose name contains it was renamed too, so the copy failed with -5102. Each site now uses DataPath::rebase(), which replaces the leading path components only. * CE-2: CropEdgeGeometry * RotateSampleRefFrame * RemoveFlaggedVertices * PadImageGeometry * CopyDataObject (the declared created paths of the copied children) * ImageGeometryCrop (CropImageGeometry and the image readers that crop) * ImageGeometryResample (ResampleImageGeom and ReadImageStack) * Add a test for each filter and a sentence to each filter doc Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* RI-3: a physical crop whose maximum equals the upper bound of the Image Geometry (origin + dimensions x spacing) computed a maximum index equal to the number of cells, so preflight failed with -5553, while a slightly larger maximum was clamped with only a warning. The shared crop helper now clamps the computed maximum index to dimensions - 1 * The helper is shared, so this fixes ReadImage, CropImageGeometry, ReadZeissTxm, ReadNIfTI and ReadMha. Add a test for each caller and a doc sentence to each filter doc Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* RI-1: a physical crop whose minimum is below the image origin passed preflight (the shared crop helper clamps it to the first cell), then execute computed a negative start index and failed with -2002. Execute now uses the same minimum rule as preflight on X, Y and Z * RI-1b: preflight discarded the warnings of the shared crop helper, so the -50503 "clamped" warning never reached the user. They are now returned with the preflight result * RI-2: "Put Input Origin at the Center of Geometry" centers the Image Geometry on (0, 0, 0) and does not use the Origin value. The behavior does not change; the parameter is renamed "Center Geometry on (0, 0, 0)", its help text and the filter doc say what it does, and a test pins it Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* RIS-1: execute looked up the physical Z crop in the already-cropped Image Geometry, so every physical Z crop that did not start at the first slice failed with -64512/-64513. Preflight and execute now compute the slice range of the full stack with one shared helper, ComputeCroppedZRange (ReadImageStackCropping.hpp) * RIS-2: a Z crop did not shift the Z origin, so the volume was placed too low. The Z origin now moves to the first kept slice, as X/Y crops and ReadImage already do * RIS-3: Convert to Grayscale on an image DataArray with 1 or 2 components was not rejected (the parameter help says it is) and read neighboring values or past the end of the buffer. Preflight now returns -23507 * Document the Z origin shift and add tests for each defect Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* FB-1: with Create Bounding Box Geometries on, the counter of written boxes was decremented before the Edge Geometry was resized, so the last bounding box was always dropped (one feature gave an empty Edge Geometry). The decrement is removed in Direct and Scanline * FB-2: the Edge Feature Ids DataArray held the box position 0, 1, 2... instead of the Feature Id of the feature each box bounds * FB-3: the internal bounds buffer was sized by the largest Feature Id, but the output loops ran over the Feature Data Attribute Matrix tuple count, which the doc allows to be larger, so the extra tuples were read past the end of the buffer. The buffer is now sized by the Feature Data tuple count, and the extra tuples are written as NaN (this also fixes minor FB-4) * CS-5: the Feature Ids parameter now declares a component shape of 1, because the algorithms read one Feature Id per element * Document the edge geometry contents and NaN tuples; add tests Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* LCS-1: preflight did not check that the Feature Ids DataArray is a
child of the Image Geometry's Cell Data Attribute Matrix. The parameter
accepts a DataArray from any Attribute Matrix, so one with fewer tuples
than the number of cells was read past its end and the values read were
used as an index for a write. Preflight now returns -3711 when the Feature
Ids parent is not the geometry's Cell Data. An Attribute Matrix only
accepts children with its tuple count, so this also guarantees the
{Z,Y,X} tuple shape that the Scanline path reads (minor LCS-3)
* The existing test fixtures now attach their Cell Data to the Image
Geometry
* Document the requirement and add a test
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* SIL-1: the Direct algorithm sized its distance table by the number of
distinct Cluster Ids but indexed it by the raw id, so sparse ids such as
{3, 7} or a negative id wrote past the end of the table. Direct now maps
Cluster Ids to dense indices (0 stays 0), as Scanline does, and rejects
negative ids with -54081
* SIL-2: with Use Mask on, preflight did not check that the Mask Array and
Cluster Ids have the same number of tuples as the Attribute Array to
Silhouette. Preflight now returns -8977
* SIL-3: for points in cluster 0, the mean distance to the point's own
cluster was a sum, so their scores were wrong. Both algorithms now
divide by the cluster size. Empty clusters cannot be the competing
cluster
* CS-9, CS-10: the Mask Array and Cluster Ids parameters now declare a
component shape of 1, because both algorithms read one value per tuple
* Document cluster 0 scoring and non-contiguous ids; add tests
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* RG-1: with Use Custom Output Type on, any Output Type for Feature Ids that differed from the Face Labels DataType made execute fail with bad_cast (-2). The write now dispatches on the output DataType and converts each slice when the types differ * RG-1 (D-A6): a Face Labels value that does not fit in the chosen output type is an execute error, -11801, that names the value range and the type. Preflight cannot see the values * Use Custom Output Type now sits directly above Output Type for Feature Ids and is linked to it, so the output type is shown only when the bool is on. Parameter keys do not change * RG-2: Face Labels of -1, the exterior label that QuickSurfaceMesh and SurfaceNets write, were copied into the Feature Ids, so the rest of a row of cells became -1. A label below 0 is now treated as 0 * Document both rules and add tests Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* QSM-2: nothing checked that each selected feature DataArray has more tuples than the largest Feature Id. The Direct algorithm read past the end of a too-short feature DataArray and returned success, while Scanline returned -62073, so the two algorithms disagreed. Direct now makes the same check and returns -62073. The check depends on the Feature Id values, so it runs at execute * Remove the misleading preflight comment that promised this check, and the unused variable next to it * Document the requirement and add a test that runs both algorithms Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* WA-1: in Multiple Files mode, two selected DataArrays with the same name in different parents (for example /A/Data and /B/Data) were written to the same output file. The second file overwrote the first and the filter reported success. Preflight now returns -51003 when two selected DataArrays have the same name, ignoring case, because some file systems do not distinguish case * Document the requirement and add a test Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* SN-1: the Direct algorithm did not check that each selected feature DataArray has more tuples than the largest Feature Id, so a too-short DataArray was read past its end. Direct now makes the same check as Scanline and returns -62073 * SN-2: preflight did not check that Cell Feature Ids and the selected cell DataArrays are children of the Image Geometry's Cell Data Attribute Matrix, so a DataArray with fewer tuples than cells could be selected and read past its end. Preflight now returns -56350 (Cell Feature Ids, or no Cell Data Attribute Matrix) and -56351 (selected cell DataArrays) * SN-4: with Apply smoothing operations on, a Max Distance from Voxel Center below 0 was undefined behavior (std::clamp with lo > hi) and a negative Relaxation Iterations was accepted silently. Preflight now returns -56352 and -56353 * Remove the unused preflight variable that suggested the SN-1 check * Document the requirements and add tests Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t data * CE-1: in Interpolate mode, when one vertex outside the crop bounds was shared by two edges that cross the bounds at different points, both edges got the same clipped vertex, so one edge was bent off its line. The outside vertex is now duplicated, one copy for each distinct clip point, and its vertex data is copied to each duplicate * CE-3: a StringArray or NeighborList in the Vertex or Edge Attribute Matrix threw bad_cast out of preflight, which crashed the application. Preflight now creates these arrays, and execute copies them with the kept vertices and edges, using the same gather list as the DataArrays * Document both behaviors and add tests Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Add IsChildOfAttributeMatrix(dataStructure, arrayPath, attributeMatrix) to DataArrayUtilities. It compares DataObject ids, so it is correct when the Attribute Matrix can be reached through more than one parent path, and it returns false when the path has no parent * ComputeLargestCrossSections (-3711) and SurfaceNets (-56350, -56351) now use it for their "DataArray must be in the Cell Data Attribute Matrix" checks instead of comparing against the first DataPath of the Attribute Matrix. Codes, messages and the no-Cell-Data guard are unchanged * Add unit tests for the helper Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* RG-4: when creating a new Image Geometry, a Spacing component of 0 or less was accepted. Preflight now returns -11802 * RG-5: preflight did not check that the Face Labels/Part Numbers DataArray is in the Triangle Geometry's Face Data Attribute Matrix, so a DataArray with fewer tuples than faces failed at execute and one with more was used silently. Preflight now returns -11803 when the Triangle Geometry has no Face Data Attribute Matrix or the DataArray is not its child (IsChildOfAttributeMatrix) * Document both rules and add tests Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* SIL-5: when only one cluster other than cluster 0 has unmasked points, b(i) is 0, so a point in that cluster scores -1 when a(i) > 0 and NaN when a(i) = 0. Masked points score 0. Finite scores lie in [-1, 1] Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* CE-5: a vertex inside the crop bounds that no kept edge uses is not copied to the created Edge Geometry Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* SN-5: Max Distance from Voxel Center is in voxel units, not physical units * SN-6: each quad is always split along its first diagonal (vertices 0 and 2); the doc said the diagonal with the smaller area was chosen Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* WA-3: a negative Maximum Tuples Per Line was accepted. Preflight now returns -51004. When the tuple count is not a multiple of the value, the last line ended with a delimiter and no newline; it now ends with a newline and no trailing delimiter (OStreamUtilities). The existing exemplar comparisons are unchanged * WA-4: Multiple Files mode with no DataArrays selected succeeded without writing anything. Both modes now return -51002 * Document both rules and add tests Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* A physical Z crop whose maximum equals the upper Z bound of the stack computed a slice index equal to the number of files and failed with -23525. The Z bounds now follow the same rule as the X/Y crop helper: a minimum below the origin is clamped to the first slice and a maximum above the bound to the last slice (both with warning -50503), and a maximum exactly at the bound selects the last slice * ComputeCroppedZRange now reuses the minimum-index computation of ComputeCroppedZDimension instead of repeating it * RIS-5: preflight kept only selected actions from its ReadImage and resample sub-preflights and dropped their warnings, so crop warnings never reached the user. Their warnings are now returned * Document the Z crop rule and add tests Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* QSM-5: -56342 was used for both a warning and an error, and -56343 and -56344 were each used for two different conditions. The node-state errors in the Scanline algorithm now have their own codes: -56346 (empty mesh), -56347 (null node-state store) and -56348 (short read). The first use of each original code is unchanged * QSM-6: when no faces were pruned, or the mesh was empty, the warning replaced the result of the winding repair, so its warnings were lost. Both algorithms now merge the two results * Add a test for the kept warnings Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This change fixes the minor findings that remained after #1765, and adds one shared check for "this DataArray is a child of this Attribute Matrix". It also corrects three filter documents. Each filter or utility has its own commit, and each behavior change has a regression test that fails before the fix. No test data archive is added or changed. 23 files changed, +462 / −47 lines.
Depends on #1765. This branch is based on
topic/batch2_defect_fixes. Merge #1765 first; until then this PR also shows the #1765 commits.Behavior changes
IsChildOfAttributeMatrixhelper. It compares DataObject ids, so a DataArray reached through a second parent path of the Cell Data Attribute Matrix is now accepted.developit finishes and writes nothing.developall values are written on one line.developit fails with −23525.developthey fail with −23522/−23523. A Z crop that does not overlap the stack still fails.No parameter keys or parameter versions change, so saved pipelines load as before. Only argument combinations that were already invalid now fail preflight.
Test plan
develop).