fix WaveformExtractor backwards compatiblity: run backwards compatiblity migration before validating metrics

[galenlynch]

For extensions whose on-disk params.json contains metric names that were renamed in 0.104 (e.g. peak_to_valley, velocity_above, l_ratio, …), validation raises a ValueError before the compat handler ever runs, which prevents loading these saved metrics.

This is because _read_old_waveforms_extractor_binary validates before migrating:

new_params = ext._set_params()                                        # 1. get defaults
updated_params = make_ext_params_up_to_date(ext, params, new_params)  # 2. merge on-disk with defaults
ext.set_params(**updated_params, save=False)                          # 3. validate + set — (errors)
if ext.need_backward_compatibility_on_load:
    ext._handle_backward_compatibility_on_load()                      # 4. migrate — never reached

Two things go wrong before the compat handler gets a chance to run:

1. Step 3: _set_params rejects deprecated names like peak_to_valley or l_ratio.
2. Step 2: make_ext_params_up_to_date strips any metric_params keys that don't exist in the new defaults: deprecated sub-params (e.g. velocity_above.min_channels_for_velocity) are silently dropped before the compat handler can migrate them to their new names (e.g. velocity_fits.min_channels).

This PR changes the order of operations so the migration handler runs first on the raw on-disk parameters, which are then pruned and validated by make_ext_params_up_to_date and set_params, respectively. Running the compat handler on the raw on-disk params first is what SortingAnalyzer already does for non-legacy folders, so the change would just make WaveformExtractor have the same logical flow.

Fixes: #4508

[galenlynch]

@chrishalcrow

[zm711]

Is there a test we could add that might help us catch a problem like this in the future. Not a simple change-detector test but a way to validate our backward compatibility better? I don't think that should slow down this PR, but I have always believed we should try to prevent these issues the best we can or we make it clear that we are having a clean break (e.g. we discussed dropping all waveform extractors at one point, but decided against it unless it has changed again).

[chrishalcrow]

Is there a test we could add that might help us catch a problem like this in the future. Not a simple change-detector test but a way to validate our backward compatibility better? I don't think that should slow down this PR, but I have always believed we should try to prevent these issues the best we can or we make it clear that we are having a clean break (e.g. we discussed dropping all waveform extractors at one point, but decided against it unless it has changed again).

Good point Zach - I've been thinking about this a bit. We could do something like: set up a GH action which makes an analyzer for each spikeinterface version, then load/tests it with the latest release. It's on my to do list, but is a bit of a project. Would be nice to set up uv+caching in the CI first, so that quickly installing 10 si versions was fast!

[chrishalcrow]

Great, thanks! LGTM and nice that the logic follows the analyzer more closely :)

[alejoe91]

LGTM too!

[samuelgarcia]

@galenlynch very good catch thank you!
You are still using the old waveforms extractor ?

[galenlynch]

We have old spike sorting results, and want to be able to read them and more recent spike sorting results in a consistent environment.