Skip to content

refactor(nn): rename discopt.nndiscopt.ml (the module is not neural-network-specific) #1219

Description

@jkitchin

Problem

python/discopt/nn/ is named for one of the four things it does. It is the ML predictor embedding + trainable surrogate layer, and its own code says so — formulations/base.py:1 reads "Formulation wrappers for embedding trained ML models into discopt", not "networks".

What actually lives there today:

Component Files Not a neural network
Decision trees / ensembles tree.py, formulations/tree_ensemble.py, TreeFormulation (formulations/base.py:12)
sklearn tree/forest/GBM readers readers/sklearn_reader.py
Trainable surrogate protocol surrogate.py (Surrogate, runtime_checkable) ✔ — explicitly admits GP/kernel means, polynomials, splines, soft trees, symbolic formulas
Kernel expansion surrogate trainable.py:430 (TrainableKernelExpansion)

The dispatcher is literally named add_predictor() (predictor.py:24), not add_network. The name misdirects readers into thinking trees and non-NN surrogates are unsupported, when they are first-class.

A second, smaller misconception the name encourages: that this module is "the MIP-representable predictors module". Only two of four encodings are MIP — relu_bigm.py and formulations/tree_ensemble.py emit binaries + big-M; full_space.py emits dm.sigmoid/tanh/softplus (full_space.py:17-22) and reduced_space.py emits nested expressions with continuous intermediates and zero binaries (reduced_space.py:115-133). MIP-representability explains why two of the files must exist; it does not describe the module.

Proposal

Rename the package to discopt.ml, keeping discopt.nn as a deprecation shim.

ml matches the existing short-subsystem naming pattern (dae, gp, llm, mo, ro) and covers both regimes the module spans — frozen predictors you optimize over, and trainable surrogates you optimize with — without claiming either. (discopt.surrogates and discopt.predictors were considered; the first breaks the short-name pattern, the second misdescribes the trainable half, since a TrainableNetwork is not a predictor you embed.)

Note discopt.gp is geometric programming, so there is no collision with a Gaussian-process reading of these names.

Blast radius (measured 2026-09-11 on main)

139 reference sites outside the package itself
Area Files
python/tests/ 13
python/discopt/ (outside nn/) 4
docs/notebooks/ 3
docs/dev/ 3
docs/design/ 1

Only one real code import exists outside the package — everything else is docstring prose:

  • python/discopt/modeling/examples.py:498-499from discopt.nn import NetworkDefinition, NNFormulation / from discopt.nn.network import Activation, DenseLayer
  • python/discopt/dae/fit.py:7,118,136 — docstring references only
  • python/discopt/ro/counterpart.py:25 — docstring reference only
  • python/discopt/skills/agents/nn-embedding-expert.md:3,8 — skill doc prose

discopt.nn is not re-exported from python/discopt/__init__.py and is not named in pyproject.toml — it is reachable only by explicit submodule import. That bounds the external break: a shim at discopt/nn/__init__.py re-exporting the public surface with a DeprecationWarning covers every downstream user.

Scope

In scope:

  1. git mv python/discopt/nn python/discopt/ml, update intra-package imports.
  2. Add python/discopt/nn/__init__.py shim: re-export the full public surface from discopt.ml, emit DeprecationWarning on import. Keep submodule paths working (discopt.nn.network, discopt.nn.formulations.base, discopt.nn.readers.*) — several tests and docs import those directly.
  3. Update the 4 in-package files, 13 test files, 3 notebooks, 4 docs, and the CLAUDE.md architecture paragraph.
  4. Add a shim regression test: import discopt.nn still works, warns, and yields objects identical to the discopt.ml ones.
  5. Rename python/discopt/skills/agents/nn-embedding-expert.md and refresh its prose (it currently describes the module as NN-only, which is the same defect at the doc layer).

Explicitly out of scope — no file reorganization. readers/ → definitions → formulations/ is a clean pipeline and the frozen/trainable split is already documented at nn/__init__.py:3-21. The one genuine structural oddity — trainable.py and surrogate.py serve dae/fit.py rather than the embedding path — has no correctness or performance defect behind it, so moving them would be a public-API break bought with an aesthetic argument. Per the repo's evidence-first rule that needs a measurement first; there isn't one. If that changes, it belongs in a separate issue, and this one should stay a pure rename.

Verification

Pure rename with a shim — no solver-core code is touched, so no bound or node-count behavior is implicated. Gates:

  • ruff check python/ + ruff format --check python/
  • mypy python/discopt/
  • pytest python/tests/test_nn_formulations.py python/tests/test_nn_equivalence.py (the equivalence harness from T-N0.1 is the real guard — it asserts the embedded formulation reproduces net.forward() / ensemble.predict())
  • pytest -m smoke
  • pytest -m slow python/tests/test_adversarial_recent_fixes.py
  • Re-execute docs/notebooks/nn_embedding.ipynb and docs/notebooks/neural_dae.ipynb, verify exit 0
  • jupyter-book build docs/ with zero warnings

Context

Came out of a design discussion about why trees and other surrogate families appear to lack module-level support. They do not lack it — it is all in nn/, under a name that hides it. Related history: #1 (the original add_predictor feature) and docs/dev/nn-module-plan.md T-N4.2, which refreshed the CLAUDE.md paragraph to list the full module surface but left the package name as-is.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions