RF Suggestion: Move FWE to FWECorrector

[adelavega]

I'm trying to determine why the FWE code lives in the Estimator class instead of FWECorrector.

I'm guessing its because the FWE code depends on if its a CBMA or IBMA, and lots of methods from self (i.e. the Estimator are used).

It might make more sense to move this code to Corrector to separate concerns.

[tsalo]

I'm not sure how the code would look, since the Monte Carlo approach typically relies on Estimator-specific methods and attributes, but it's definitely worth looking into.

[tsalo]

Here is a breakdown of the current situation:

• CBMAEstimator has a correct_fwe_montecarlo method by which to apply the Monte Carlo FWE correction.
  • ALE, MKDADensity, and KDA all use this method directly.
  • SCALE overrides the method in order to raise a NotImplementedError.
• PairwiseCBMAEstimator doesn't directly override the correct_fwe_montecarlo method, but it has two child classes that do:
  • MKDAChi2 overrides the method with its own version.
  • ALESubtraction overrides the method, but only to raise a NotImplementedError.
• The IBMA Estimators, which inherit from MetaEstimator, do not have correct_fwe_montecarlo at all.
  • Actually, PermutedOLS (which uses nilearn's permuted_ols) has a correct_fwe_montecarlo method, which just reruns the function with the permutation procedure enabled.

I'm not sure how to handle the Estimator-specific overrides if we move the correction code to the Corrector class. Do you have any ideas?

I guess we could write Estimator-specific methods within FWECorrector (e.g., _montecarlo_density, _montecarlo_chi2, _montecarlo_permuted_ols, etc.) and select the appropriate method within .transform. We'd probably have to hardcode the Estimator names into the Corrector though.

[adelavega]

Yes, I was looking at that, and realized that's the situation.

Still feels odd because this transfer is really doing very different things depending on the situation, so it's already got if/then clauses within it.

Maybe there's common logic that can be extracted from the Estimators into the Corrector, while still leaving estimator specific logic in the Estimator that would trigger not implemented errors, or Estimator specific logic.

[adelavega]

As an aside, the way the montecarlo methods are written it's also looking tricky to optimize w/ Numba, since the repeated portion is full of Python objects and non-numerical computation.

It would probably be good to clean that up and save as much of the computation into one burst so that can be numerically optimized. There is probably a lot of Python overhead that is being repeated over and over, which even w/o Numba optimization will be the cause of performance issues.