Python-side dimension-limit checks, large-dimension tests, and 1-column igd support (#72)#73
Open
blankjul wants to merge 3 commits into
Open
Python-side dimension-limit checks, large-dimension tests, and 1-column igd support (#72)#73blankjul wants to merge 3 commits into
blankjul wants to merge 3 commits into
Conversation
Add checks to the Python wrappers so that inputs beyond what the C library supports are rejected with a clear ValueError instead of reaching C as undefined behavior (multi-objective#72): - 255 columns (the range of dimension_t) for igd, igd_plus, avg_hausdorff_dist, epsilon_additive, epsilon_mult, pareto_rank, is_nondominated and any_dominated. Before this, 256 columns segfaulted via dimension_t wraparound. - 31 columns for hypervolume (function and class), hv_contributions and hv_approx. Before this, 32 columns silently returned a wrong value (or crashed, depending on the compiler). - at least 2 columns for epsilon_additive/epsilon_mult: their C kernel is unrolled over the first two dimensions, so 1-column input read out of bounds and returned garbage. Add test_dimension_limits.py covering large-dimension correctness (32-255 columns, verified against independent numpy references) and the error behavior at each limit.
The gd_common_helper_ loop is fully general in dim, so the 2 <= dim lower bound is purely conservative: with 1 <= dim, igd, igd_plus and avg_hausdorff_dist compute exact results for 1-column input (tests included) and higher dimensions are unchanged. This case is exercised in the wild: pymoo 0.6.2 calls moocore.igd on design-space data, so any single-variable problem passes a 1-column array. Legalizing it keeps those installs working; rejecting it would break them from a bugfix release. Note this deliberately does not touch epsilon.h: its kernel is unrolled over the first two dimensions, so 2 <= dim is load-bearing there (1-column input is now rejected in Python instead).
for more information, see https://pre-commit.ci
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
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.
Closes the remaining checklist items from #72.
Commit 1 — Python-side dimension-limit checks + tests
Adds a
_check_nobj()helper and per-functionValueErrors so that inputs beyond what the C library supports are rejected in Python instead of reaching C as undefined behavior:igd,igd_plus,avg_hausdorff_dist,epsilon_additive,epsilon_mult,pareto_rank,is_nondominated,any_dominateddimension_t)dimension_twraparound)hypervolume(function + class),hv_contributions,hv_approxepsilon_additive,epsilon_multNew
test_dimension_limits.py:igd,igd_plus,epsilon_additive,pareto_rank,is_nondominated— verified against independent pure-numpy references (~0.3 s total),Note on epsilon: while testing the lower bound we found that
epsilon_helper_is unrolled over the first two dimensions (MAX(eps_value_agree_(0), eps_value_agree_(1))unconditionally), so a 1-column input performs an out-of-bounds read and returns garbage — e.g.epsilon_additivereturned0.616where the correct value is0.009(formula cross-checked against moocore at 2–3 columns). Reachable from Python in all released versions; now rejected with a clear error.Commit 2 — relax
igd.htoASSUME(1 <= dim)(drop if you disagree)The
gd_common_helper_loop is fully general indim, so the2 <= dimlower bound is purely conservative there: with1 <= dim,igd/igd_plus/avg_hausdorff_distcompute exact results for 1-column input (tests included intest_dim1_metrics.py) and higher dimensions are unchanged.Why legalize rather than reject: pymoo 0.6.2 (current release) calls
moocore.igdon design-space data, so any single-variable problem passes a 1-column array in production. Today that violates theASSUME(it happens to work on GCC release builds but would assert underDEBUG=1). AValueErrorhere would retroactively break those installs; relaxing the assume makes the existing behavior well-defined.epsilon.his deliberately untouched — its2 <= dimis load-bearing (see above).This is a separate commit so it's easy to drop if you'd rather handle the lower bound differently.
Verification
test_dimension_limits.py40,test_dim1_metrics.py4)Open questions (not in this PR)
_moocore.py? Happy to change.igdwith NaN silently returns0.0today. A SciPy-style opt-outcheck_finite=Truekwarg would avoid always paying the extra pass — can do as a follow-up if you want it.Downstream context: anyoptimization/pymoo#793.