Skip to content

Commit 053becf

Browse files
author
Yuma Ichikawa
committed
chore: senior-eng audit fixes (datasets, sa docs, viz heatmap, public api)
Surgical fixes from a top-to-bottom code audit. No behaviour change for working code paths; each fix repairs a real corner case the audit found. datasets._default_data_dir Wheel installs from PyPI have no repo above ``site-packages/qqa/``, so ``parents[2] / "data"`` resolved to a non-existent path and the loaders crashed deep inside ``os.listdir`` with no actionable hint. Now: prefer ``QQA_DATA_DIR``, fall back to the source-tree path when it exists, then ``Path.cwd() / "data"``. ``_resolve`` raises a FileNotFoundError with the remediation when the directory is missing. sa.simulated_annealing docstring Used to claim BinaryInstanceRelaxation was supported, but the function rejects batched-instance problems with NotImplementedError. Docstring now matches the implementation. visualization.plot_solution_heatmap Square (N, N) one-hot solutions (TSP, QAP, NQueens) used to silently collapse to row 0 when the caller did not pass ``problem``. Show the full matrix in that fall-through branch instead. tests/test_public_api Listed only the pre-0.4 names. Extend with PSpinGlass / RandomFieldIsing / MinimumDominatingSet / SAResult / enable_tf32 / polish / simulated_annealing / warmstart / *Instance, and add one mechanical test that ``__all__`` ⊆ what is actually importable. Made-with: Cursor
1 parent 4d4f98a commit 053becf

4 files changed

Lines changed: 77 additions & 9 deletions

File tree

src/qqa/datasets.py

Lines changed: 33 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,20 +33,49 @@
3333

3434

3535
def _default_data_dir() -> Path:
36+
"""Resolve the on-disk benchmark dataset directory.
37+
38+
Priority order:
39+
1. ``$QQA_DATA_DIR`` if set — explicit user override.
40+
2. ``<repo_root>/data`` if this module lives inside the source tree
41+
(``src/qqa/datasets.py`` => ``parents[2] == repo_root``).
42+
3. ``./data`` next to the current working directory — a sensible
43+
fallback for wheel installs from PyPI where no source tree exists.
44+
45+
Loaders raise a clear ``FileNotFoundError`` when the resolved
46+
directory does not contain the requested benchmark, so callers always
47+
get an actionable message ("set QQA_DATA_DIR or pass path=") rather
48+
than an opaque ``listdir`` error.
49+
"""
50+
3651
env = os.environ.get("QQA_DATA_DIR")
3752
if env:
3853
return Path(env).expanduser().resolve()
39-
# src/qqa/datasets.py -> repo_root = parents[2]
40-
return _THIS.parents[2] / "data"
54+
# When installed in editable / source mode, ``parents[2]`` is the
55+
# repository root and ``parents[2] / "data"`` ships ``mis/er-small``.
56+
repo_data = _THIS.parents[2] / "data"
57+
if repo_data.is_dir():
58+
return repo_data
59+
# Wheel install fallback — let the user opt in via cwd/data.
60+
return Path.cwd() / "data"
4161

4262

4363
DATA_DIR: Path = _default_data_dir()
4464

4565

4666
def _resolve(path: str | os.PathLike | None, default_subpath: str) -> Path:
4767
if path is not None:
48-
return Path(path).expanduser().resolve()
49-
return _default_data_dir() / default_subpath
68+
resolved = Path(path).expanduser().resolve()
69+
else:
70+
resolved = _default_data_dir() / default_subpath
71+
if not resolved.is_dir():
72+
raise FileNotFoundError(
73+
f"Benchmark directory {resolved!s} does not exist. "
74+
"Set the QQA_DATA_DIR environment variable to point at the "
75+
"QQA4CO repository's ``data/`` directory, or pass an explicit "
76+
"``path=`` argument to this loader."
77+
)
78+
return resolved
5079

5180

5281
def _load_pickle(p: Path):

src/qqa/sa.py

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -109,9 +109,15 @@ def simulated_annealing(
109109
Parameters
110110
----------
111111
problem:
112-
Any :class:`~qqa.problems.COProblem` exposing ``loss_fn(x)`` and a
113-
``relaxation`` attribute (BinaryRelaxation, BinaryInstanceRelaxation,
114-
or SpinRelaxation). CategoricalRelaxation is not yet supported.
112+
Any single-instance :class:`~qqa.problems.COProblem` exposing
113+
``loss_fn(x)`` and a ``relaxation`` attribute
114+
(:class:`~qqa.relaxation.BinaryRelaxation` or
115+
:class:`~qqa.relaxation.SpinRelaxation`).
116+
:class:`~qqa.relaxation.CategoricalRelaxation` and batched-instance
117+
problems (those exposing ``num_instance``) are rejected at the API
118+
boundary with :class:`NotImplementedError`; iterate over instances
119+
and call :func:`simulated_annealing` per instance, or use
120+
:func:`qqa.anneal` which handles both natively.
115121
sol_size:
116122
Number of independent SA chains run in parallel on GPU.
117123
num_sweeps:

src/qqa/visualization.py

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -602,18 +602,23 @@ def plot_solution_heatmap(
602602
sol = sol.detach().cpu().numpy()
603603
sol = np.asarray(sol)
604604
if sol.ndim == 2:
605-
# Two valid 2-D shapes reach here:
605+
# Three valid 2-D shapes reach here:
606606
# * batched-instance problems: ``(num_instance, max_node)`` — keep
607607
# the whole matrix so each row is one instance's solution.
608608
# * single-instance categorical problems: ``(N, K)`` — collapse to
609609
# the chosen category per variable via argmax so the heatmap
610610
# shows a single row of class indices.
611+
# * permutation problems (TSP, QAP, NQueens) where ``best_sol`` is
612+
# a square one-hot ``(N, N)`` matrix — show the full matrix so
613+
# the assignment structure is visible. When ``problem`` is not
614+
# provided we still show the full matrix rather than silently
615+
# collapsing to the first row.
611616
if problem is not None and getattr(problem, "num_instance", None) is not None:
612617
arr = sol
613618
elif problem is not None and getattr(problem, "num_category", None) is not None:
614619
arr = np.argmax(sol, axis=1)[None, :].astype(float)
615620
else:
616-
arr = sol[0][None, :]
621+
arr = sol
617622
elif sol.ndim == 1:
618623
arr = sol[None, :]
619624
else:

tests/test_public_api.py

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,18 @@
6464
"PopulationTracker",
6565
"TrajectoryTracker",
6666
"Relaxation",
67+
# Post-0.4.0 additions.
68+
"MinimumDominatingSet",
69+
"PSpinGlass",
70+
"RandomFieldIsing",
71+
"SAResult",
72+
"enable_tf32",
73+
"polish",
74+
"simulated_annealing",
75+
"warmstart",
76+
"MaxCliqueInstance",
77+
"MaxCutInstance",
78+
"MaximumIndependentSetInstance",
6779
]
6880

6981

@@ -73,6 +85,22 @@ def test_top_level_export_exists(name: str) -> None:
7385
assert name in qqa.__all__, f"qqa.{name} must be listed in qqa.__all__"
7486

7587

88+
def test_public_api_set_matches_dunder_all() -> None:
89+
"""``__all__`` must agree with what is reachable as ``qqa.<name>``.
90+
91+
Catches the common slip of adding a class to ``__all__`` while
92+
forgetting to import it (or vice versa).
93+
"""
94+
95+
advertised = set(qqa.__all__) - {"__version__"}
96+
actually_present = {n for n in advertised if hasattr(qqa, n)}
97+
missing = sorted(advertised - actually_present)
98+
assert not missing, (
99+
f"{missing!r} is listed in qqa.__all__ but not importable from "
100+
"the top level — fix the import in src/qqa/__init__.py."
101+
)
102+
103+
76104
def test_callback_re_export_is_the_same_object() -> None:
77105
"""The re-exports must be identical objects, not look-alike copies."""
78106
from qqa.callbacks import (

0 commit comments

Comments
 (0)