Skip to content

Fix sample sorting algorithms - #261

Merged
yoelcortes merged 1 commit into
masterfrom
qsdsan
Oct 11, 2026
Merged

yoelcortes merged 1 commit into
masterfrom
qsdsan

Conversation

@yalinli2

Copy link
Copy Markdown
Member

Model.load_samples(samples) sorts samples with a nearest-neighbour walk over the coupled-parameter columns. Two inputs made that sort crash. sort=False avoided both.

1. Samples that share the same coupled-parameter values

Raised AssertionError: issue in sorting algorithm (or KeyError) when several samples have identical values for all coupled parameters and differ only in isolated parameters. Morris and Sobol designs produce many such samples. I saw it with Morris N >= 20 and Sobol N = 256, on models with 6 to 11 parameters of which 1 to 3 are coupled.

Cause: the walk assumed argsort puts each row itself at position 0 of its neighbour list, and scanned from position 1. With ties at distance 0, another duplicate can land at position 0 and is never considered. If it is the last remaining sample, the loop makes no progress and the assertion fires. The initial step (nearest_arr[0, 1]) had the same flaw and could pick row 0 itself, raising KeyError.

Fix:

  • Scan neighbours from index 0 instead of 1. The current sample is already removed from remaining, so this is safe.
  • Start the walk at sample 0 and let the loop pick its nearest neighbour, replacing the special first step.

2. A coupled parameter that is constant across all samples

Raised FloatingPointError (when numpy errors are set to raise; otherwise the distances became NaN). The column has zero range, so normalizing (samples - min) / (max - min) divided by zero.

Fix: replace zero ranges with 1 before normalizing, so a constant column normalizes to zeros.

The result is still a greedy nearest-neighbour path.

@yoelcortes yoelcortes left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvements. Reading the code, it looks straight forward. But just to make sure, I also checked the outputs with the old version and they match.

@yoelcortes
yoelcortes merged commit ec51ac4 into master Oct 11, 2026
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants