[REF] CBMA re-organization and improvement #393
Conversation
Codecov Report
@@ Coverage Diff @@
## master #393 +/- ##
==========================================
- Coverage 82.57% 82.32% -0.25%
==========================================
Files 38 40 +2
Lines 3913 3848 -65
==========================================
- Hits 3231 3168 -63
+ Misses 682 680 -2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I think the code itself looks good, although there are a few docstrings that I'd like to fill out with an eye toward explaining the inheritance structure.
One thing that I think needs to be done before this is merged is updating the API section of the documentation and checking the docs build. If you'd rather not do that, I can handle it in a follow-up PR.
|
I've opened #395 targeting the |
nimare/meta/cbma/base.py
Outdated
| def _compute_weights(self, ma_values): | ||
| return None |
There was a problem hiding this comment.
Do we need this if it isn't an abstract method?
There was a problem hiding this comment.
Yes, because it serves as a dummy method for subclasses that don't take weights (e.g., ALE). It just allows us to avoid having different _fit() methods.
nimare/meta/cbma/base.py
Outdated
| def _compute_summarystat(self, ma_values): | ||
| pass |
There was a problem hiding this comment.
Should this be an abstract method?
There was a problem hiding this comment.
It will be, but only once we've refactored MKDAChi2 to have the same general architecture, which I'm planning to do separately. Otherwise it will break that class. (I suppose we could define a placeholder method in MKDAChi2 if you like.)
| LGR = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class CBMAEstimator(MetaEstimator): |
There was a problem hiding this comment.
This probably shouldn't happen now, but I'm thinking that we may want to further divide this into a DensityBasedEstimator and something else, so separate density-based approaches like ALE and MKDA from dramatically different ones like SDM-PSI.
There was a problem hiding this comment.
Agreed. I initially wanted to do it that way (i.e., to put all the abstracted methods into a DensityCBMAEstimator class), but figured we should see how far we can get by consolidating everything, instead of multiplying classes in advance. If we can't re-use everything in there right now for the pairwise estimators, we may also need to define a separate OneSample class (to parallel the Pairwise subclass).
|
@tsalo I added some docstrings. Assuming this looks good, I think we're okay to merge, unless you have other comments. |
This PR refactors and standardizes the CBMA hierarchy. Changes include:
cbmafolder containingbase,ale, andmkdamodules.metanamespace (though importing through the individual submodules will also still work, so code shouldn't break).ALEandMKDADensityclasses have been dramatically stripped down, with most of the common functionality moving intoCBMAEstimator.MKDADensity, and should be considerably faster, as it doesn't need to compute p-values in each permutation.null_to_p; now only processes unique values in array, and then reconstructs the original.p_to_summarystat()classmethod inCBMAEstimatorthat looks up a critical summary stat value based on the passed p-value.With this done, we're in a good position to refactor some of the paired test classes, which could also in theory use a lot of the same machinery. But let's keep that for a separate PR.