Revise communication RE linear robust estimator

[Lestropie]

Closes #2586.

Claude agreed with my diagnosis and came up with an intervention that was the same as my own.

I don't know whether the imprecision of the implementation was influenced at all by uncertainty about whether the L2 robust estimator and "no robust estimator" are equivalent; which Claude says they are. So removing "None" as an option and just having a "choice of estimator" instead of "one of these robust estimators, or none" is also an option.

---

Full log of Claude query and response

❯ Analyse the conflict described in GitHub Issue #2586. Cross-check the
behaviour within population_template and mrregister. Report on whether the
posted interpretation is correct, and what change to the code or
documentation would be most appropriate.

The interpretation is correct. The current behaviour (still present on master)
is:

mrregister (cpp/mrregister.cpp:433–446, 847–887)

• If -rigid_metric.diff.estimator is omitted, rigid_estimator is initialised
  to Registration::None and the code path that runs is Metric::MeanSquared /
  MeanSquared4D — i.e. ordinary least squares, not the DifferenceRobust
  branch.
• The help text in src/registration/linear.cpp:259-264 (and the affine
  equivalent at :327-332) advertising "Default: l2" is therefore misleading: the
  explicit l2 choice triggers a different code path (DifferenceRobust) that
  is never visited under default invocation.
• Mathematically, the two paths are equivalent (Metric::L2 in
  src/registration/metric/robust_estimators.h:53 is just x*x / slope x,
  identical in form to OLS), so end-user numerical behaviour is the OLS the help
  text implies — but the documented "default" is not actually executed.

population_template (bin/population_template:56, 659–664, 1172–1173,
1186–1187)

• Default behaviour: app.ARGS.linear_estimator is None, the truthiness checks
  at :1172 / :1186 fail, and the -rigid_metric.diff.estimator /
  -affine_metric.diff.estimator option is not forwarded to mrregister. So
  mrregister falls through to its MeanSquared path. The advertised "Default:
  None (no robust estimator used)" is faithful.
• Note a latent inconsistency at :660: the validator accepts the literal
  string none (case-insensitive), but the dispatch at :1172/:1186 uses bare if
  linear_estimator:, which is truthy for "none". Passing -linear_estimator none
  would forward -rigid_metric.diff.estimator none to mrregister, whose
  type_choice only accepts {l1, l2, lp} and would error.

Recommended changes

1. src/registration/linear.cpp (preferred fix) — correct the help text on both
   rigid and affine metric.diff.estimator options. Remove "Default: l2"; replace
   with something like "Default: not used (ordinary least squares / mean squared
   difference is applied; mathematically equivalent to l2)". This keeps current
   numerical behaviour and accurately reflects that the option only takes effect
   when explicitly specified.
2. Make none an explicit choice (your stated plan) in both interfaces. In
   mrregister, add it to linear_robust_estimator_choices and treat it the same as
   the option being absent. In population_template, this avoids the
   validator/dispatch mismatch noted above. While doing so, also fix the dispatch
   guards at :1172/:1186 to if linear_estimator and linear_estimator.lower() !=
   'none': so an explicit none is suppressed rather than forwarded.
3. Not recommended: changing the default path in mrregister to actually route
   through DifferenceRobust. It's an extra layer of indirection for an
   OLS-equivalent computation and would silently change anyone relying on the
   current MeanSquared code path; the documentation fix is the right call.

❯ Proceed with recommended changes 1 and 2. Change the command-line options for
robust estimators to mrregister to include "none" as an available choice.
Change population_template to use the "enum" module rather than strings for
handling user-specified & default robust estimators.

● All changes are in place and verified. Summary:

src/registration/linear.cpp

• Added "none" to linear_robust_estimator_choices.
• Replaced the misleading "Default: l2" help text on both
  rigid_metric.diff.estimator and affine_metric.diff.estimator with an accurate
  description: explicit none choice that documents the actual behaviour (mean
  squared difference / ordinary least squares, mathematically equivalent to l2),
  and Default: none.

cmd/mrregister.cpp

• Added case 3 to both rigid and affine estimator dispatch switches, mapping
  the new "none" choice to Registration::None (the same path already taken when
  the option is omitted).

bin/population_template

• Added import enum and a LinearEstimator(enum.Enum) with members
  NONE/L1/L2/LP plus a DEFAULT_LINEAR_ESTIMATOR constant.
• The -linear_estimator argument now uses argparse choices= and default=
  driven by the enum, with a help string aligned to mrregister's.
• Replaced string-based parsing/validation with
  LinearEstimator(app.ARGS.linear_estimator) and is not LinearEstimator.NONE
  checks; this also fixes the latent bug where passing -linear_estimator none
  would forward the unsupported string to mrregister.
• Both dispatch sites (rigid + affine) now pass linear_estimator.value only
  when the estimator is not NONE.

Verification: ./check_syntax --diff and ./run_pylint both pass. Auto-generated
docs under docs/reference/commands/ will refresh on the next
./docs/generate_user_docs.sh run.

[jdtournier]

I have to admit I'm also confused about the difference between L2 and none - if they both reduce to mean squared difference, I don't get the rationale for even listing them as separate entities... I'll need to dig into the code to get a proper sense of whether there really is a difference, and if so what that difference really is...

[Lestropie]

OK sent Claude on a deep dive. Story basically conforms to my hypothesis.

1. In their current implementation:
   1. MeanSquared and DifferenceRobust with L2 yield exactly the same result.
   2. MeanSquared was reported (5623b12) to be 5% slower; but with modern compilers this might entirely vanish (measured 0.5% in debug mode).
2. In future implementations:
   Plans for "robust" estimators to not be exclusively about "range of M-estimators", but to also include outlier rejection. There's unused code in there intended for this purpose.

Question then is what to change.

It agrees with my instinct (though acknowledge they can be hesitant to do the converse): the overall cleanest solution would be to remove MeanSquared(4D), and rename the "robust" estimators (in both code and CLI) to simply be norm / loss / linear / M-estimators. If there's at some point functionality added to that code over and above the exponent that genuinely makes the estimators "robust", then that can be an independent dial with its own interface (even templated if we really want to preserve speed of the default mean squared case).

I'd prefer however to do any refactoring on dev, and minimise master changes to just resolving ambiguity, preferably as much as possible prioritising documentation, then interface, then code changes. I can probably modify what's here slightly to be more content, which will probably be reverting some changes and doing the best I can in documentation.

[Lestropie]

@jdtournier See if the overall revised proposal make sense to you.

Changes compared to current master code:

• No longer refer to "Default: None" when no choice "None" exists; the text was intended to refer to the absence of the command-line option.
• Dedicate paragraph in mrregister to explaining the distinction (better than bloating the option section of the help page, and also it applies equally to two options).
• Be explicit about (current) equality of "Robust L2" versus "No robust estimator".
• Variable names better reflect "a choice of one of the "robust" estimators" rather than being "the" linear estimator.
• Carry the absence of a robust estimator selection through use of a std::unique_ptr (std::optional would be better on dev), rather than devoting a "None" enum choice (which was also unscoped). This also removes a non-correspondence between the std::vector<std::string> used to construct the command-line options and the corresponding enumeration.
• mrregister no longer erroneously claims that by default it invokes the "robust" l2 metric. Even though it currently behaves identically to the mean squared difference metric actually invoked in the absence of that option, it's nevertheless not faithful to what's invoked.