Improve the inheritance and consistency of trainers

[yishaishimoni]

What should we add?

The various trainers in this project inherit from BaseTrainer, but have a different signature for their train() function.
This can cause issues at runtime, but more importantly it creates confusion for users regarding how to use each of the trainers.

There are even cases where different trainers require the same input but with a different name (e.g., reps vs. qaoa_depth or params0 vs previous_optimal_point).

There are several ways to address this:

1. It is possible to define a single minimal set of inputs to train() and make sure that all other parameters are entered at __init__. This is a reasonable solution when the additional parameters really inherently define the trainer and not the specific training run.
2. Perhaps a set of trainers can be identified that share similar inputs and can then be bunched together in a more coherent inheritance hierarchy. For example, trainers that support initial point can all inherit from a FromInitTrainer (or some other, probably better ;) , name)

Each of these solutions, and even just merging parameters that mean the same with different names, will be backwards incompatible and so require some thought.

Below I provide a table of the parameters each of the classes receives in their train function:

Trainer Class	cost_op	mixer	initial_state	ansatz_circuit	params0	reps	degree	folder	file_pattern	parameter_ranges	num_points	lower_bound	upper_bound	seed	previous_optimal_point	trainer1_kwargs	initial_dt	qaoa_depth
BaseTrainer	✓	✓	✓	✓	✓	-	-	-	-	-	-	-	-	-	-	-	-	-
FixedAngleConjecture	✓	~	~	~	~	✓	✓	-	-	-	-	-	-	-	-	-	-	-
OptimizedParametersLoader	~	~	~	~	~	-	-	✓	✓	-	-	-	-	-	-	-	-	-
DepthOneScanTrainer	✓	✓	✓	✓	~	-	-	-	-	✓	✓	-	-	-	-	-	-	-
DepthOneGammaScanTrainer	✓	~	✓	✓	~	-	-	-	-	✓	✓	-	-	-	-	-	-	-
QAOAPCA	✓	✓	~	✓	✓	-	-	-	-	-	-	-	-	-	-	-	-	-
RandomPoint	~	~	~	~	~	✓	-	-	-	-	-	✓	✓	✓	-	-	-	-
RecursionTrainer	✓	✓	✓	✓	✓	✓	-	-	-	-	-	-	-	-	-	-	-	-
RecursiveTransitionStates	✓	✓	✓	✓	~	✓	-	-	-	-	-	-	-	-	✓	-	-	-
ReweightingTrainer	✓	✓	✓	✓	✓	-	-	-	-	-	-	-	-	-	-	✓	-	-
ScipyTrainer	✓	✓	~	✓	✓	-	-	-	-	-	-	-	-	-	-	-	-	-
TQATrainer	✓	✓	✓	✓	✓	~	-	-	-	-	-	-	-	-	-	-	✓	-
TransferTrainer	✓	x	x	x	~	-	-	-	-	-	-	-	-	-	-	-	-	✓
TransitionStatesTrainer	✓	✓	✓	✓	~	-	-	-	-	-	-	-	-	-	✓	-	-	-

Where

• ✓ means that the parameter is accepted and used
• - means that the parameter is not accepted in the API
• x means that the parameter is accepted in the API but an exception is thrown if actually passed
• ~ means that the parameter is accepted but not used

[yishaishimoni]

From the above table it is clear that FixedAngleConjecture, OptimizedParametersLoader, RandomPoint, and TransferTrainer should not actually inherit from BaseTrainer. This is actually consistent with the fact that they are not really trainers, in the sense that there is no optimization done. As a result they also do not need self._evaluator other than to return the energy in the ParamResults (which only FixedAnglesConjecture` does so there's also an inconsistency here).

The reason for this unconventional inheritance is given in the docstring for OptimizedParametersLoader, which states "In a slight abuse of notation, this class is not really a trainer. However,
having it inherit from the BaseTrainer ensures that it is usable in the
train pipeline."

I would claim that this is inaccurate. These classes cannot be used in general in a pipeline, but can only ever come first in the pipeline. They are in fact parameter loaders and not trainers.
This insight hints that an OptimizationPipeline object should accept a parameter loader/setter, followed by a list of trainers, each one potentially with its own evaluator.

In turn, this hints that some of the parameters, such as reps, should not be given in the train function but at initialization.
For example for a non d-regular graph, you would start with a FixedAngleConjecture to load the angles, and then define a pipeline that starts with a DepthOneScanTrainer, followed by a RecursionTrainer that is pre-set to increase from p=1 to p=n.

I believe num_points and parameter_ranges can also be defined at initialization.

In summary, here is my suggestion:

1. Change qaoa_depth to reps, previous_optimal_point to params0, initial_dt to params0
2. Either create a completely different class for FixedAngleConjecture, OptimizedParametersLoader, RandomPoint, and TransferTrainer or create a minimal abstract ParamsLoader that BaseTrainer inherits from. These classes do not accept mixer, initial_state, 'ansatz_circuit, params0`.
3. The rest of the classes ONLY accept these params in their train() function, where everything else, namely reps, degree, folder, file_pattern, parameter_ranges, num_points, lower_bound, upper_bound, seed, and trainer1_kwargs , is defined in the relevant initializations.

[yishaishimoni]

Feedback from Bob on Trainer Refactoring Proposal

Overall Assessment: Feasible with Significant Benefits ✅

Your analysis is thorough and the proposal addresses real architectural issues. The refactoring is feasible but requires careful planning due to backward compatibility concerns.

---

✅ Strengths of the Proposal

1. Correct Identification of Parameter Loaders vs. Trainers

You're absolutely right that FixedAngleConjecture, OptimizedParametersLoader, RandomPoint, and TransferTrainer are not trainers in the optimization sense. They:

• Don't perform optimization
• Don't need self._evaluator (except FixedAngleConjecture optionally)
• Cannot meaningfully implement minimization property
• Can only appear first in a pipeline

The current inheritance is indeed an "abuse of notation" as acknowledged in OptimizedParametersLoader.

2. Parameter Naming Inconsistencies

The inconsistencies you identified are real problems:

• reps vs. qaoa_depth (same concept, different names)
• params0 vs. previous_optimal_point (same concept, different names)
• initial_dt is only used by TQATrainer and should be params0

3. Configuration vs. Runtime Parameters

Your insight about moving reps, degree, parameter_ranges, num_points, etc. to __init__ is sound. These define the trainer's behavior, not the specific training run.

---

⚠️ Challenges & Considerations

1. Pipeline Mechanism Dependency

The current train.py pipeline assumes all components inherit from BaseTrainer:

• Line 252: trainer = trainer_cls.from_config(conf["trainer_init"])
• Line 272: result = trainer.train(input_problem, **train_kwargs)

Impact: Creating a separate ParamsLoader class requires modifying the pipeline to handle two types of components.

2. Result Chaining Logic

Lines 258-263 in train.py show how results chain between trainers:

if len(result) > 0 and "result" in train_kwargs:
    for result_key, arg_name in train_kwargs["result"].items():
        train_kwargs[arg_name] = result[result_key]

This mechanism allows params0 to be extracted from previous results. Separating loaders from trainers needs to preserve this.

3. Backward Compatibility

Existing config files (e.g., data/methods/train_method_*.json) will break. You'll need:

• Migration scripts
• Deprecation warnings
• Version detection

---

💡 Recommended Approach

Option A: Minimal Disruption (Recommended)

Create a clear hierarchy while maintaining backward compatibility:

BaseComponent (abstract)
├── ParamsProvider (abstract)
│   ├── FixedAngleConjecture
│   ├── OptimizedParametersLoader
│   ├── RandomPoint
│   └── TransferTrainer
└── BaseTrainer (abstract)
    ├── ScipyTrainer
    ├── RecursionTrainer
    ├── DepthOneScanTrainer
    └── ... (other optimizing trainers)

Benefits:

• Clear semantic separation
• Both inherit from BaseComponent with common interface
• Pipeline can handle both types uniformly
• Gradual migration path

Implementation:

1. Create BaseComponent with minimal interface: train(), from_config(), to_config(), parse_train_kwargs()
2. Make BaseTrainer inherit from BaseComponent and add optimization-specific methods
3. Create ParamsProvider inheriting from BaseComponent
4. Migrate the four classes to ParamsProvider
5. Update pipeline to accept both types

Option B: Explicit Pipeline Structure

Modify train.py to explicitly support:

{
  "params_provider": {...},  # Optional, runs first
  "trainer_chain": [...]     # Actual trainers
}

Benefits:

• Makes the distinction explicit in config
• Clearer semantics
• Easier validation

Drawbacks:

• More invasive changes to pipeline
• All configs need updating

---

📋 Specific Recommendations

1. Parameter Standardization (High Priority)

Standardize on these names:

• reps → Keep as reps (more common in codebase)
• qaoa_depth → Rename to reps
• params0 → Keep as params0 (standard optimization term)
• previous_optimal_point → Rename to params0
• initial_dt → Rename to params0

2. Move to __init__ (Medium Priority)

These should be constructor parameters:

• reps (for trainers that need fixed depth)
• degree (for FixedAngleConjecture)
• parameter_ranges, num_points (for DepthOneScanTrainer)
• lower_bound, upper_bound, seed (for RandomPoint)
• folder, file_pattern (for OptimizedParametersLoader)

Keep in train():

• cost_op (always runtime)
• mixer, initial_state, ansatz_circuit (problem-specific)
• params0 (can come from previous trainer)

3. Minimal train() Signature (High Priority)

def train(
    self,
    cost_op: SparsePauliOp,
    mixer: QuantumCircuit | None = None,
    initial_state: QuantumCircuit | None = None,
    ansatz_circuit: QuantumCircuit | None = None,
    params0: list[float] | None = None,
) -> ParamResult:

All trainers should accept this. Additional parameters go in __init__.

---

🚨 Potential Drawbacks

1. Breaking Changes

• All existing configs need updating
• User code calling trainers directly will break
• Documentation needs comprehensive updates

2. Migration Complexity

• Need to support both old and new APIs during transition
• Deprecation warnings for 1-2 releases
• Clear migration guide required

3. Testing Burden

Looking at test/training/, there are extensive tests that will need updates:

• test_fixed_angles.py
• test_parameter_loader.py
• test_scanner.py
• test_recursion.py
• And more...

---

✨ Better Solution?

Your proposal is sound, but I'd suggest Option A (Minimal Disruption) with these modifications:

1. Phase 1: Parameter standardization only

   • Rename qaoa_depth → reps, previous_optimal_point → params0
   • Add deprecation warnings for old names
   • Update all internal code
   • Low risk, high value

2. Phase 2: Move config params to __init__

   • Gradually move trainer-specific params to constructors
   • Maintain backward compatibility via from_config()
   • Medium risk, high value

3. Phase 3: Create ParamsProvider hierarchy

   • Extract the four classes to new base
   • Update pipeline to handle both
   • Higher risk, but cleaner architecture

This phased approach allows you to:

• Deliver value incrementally
• Test each phase thoroughly
• Maintain backward compatibility longer
• Abort if issues arise

---

🎯 Conclusion

Is it feasible? Yes, absolutely.

Are there drawbacks? Yes, but manageable with phased approach.

Is there a better solution? Your solution is good; I recommend the phased implementation above to reduce risk.

Key Success Factors:

1. Comprehensive test coverage before starting
2. Deprecation warnings, not immediate breaks
3. Clear migration documentation
4. Phased rollout over 2-3 releases

The refactoring will significantly improve code clarity and maintainability. The main challenge is managing the transition without breaking existing users.