Add Fixed Effects Meta-Analysis with Hedges’ g #894
Conversation
JulioAPeraza
added
enhancement
Reviewer's Guide by SourceryThis pull request introduces a new Fixed Effects Meta-Analysis using Hedges' g. The implementation includes a new estimator class FixedEffectsHedges in nimare/meta/ibma.py, along with necessary transformations in nimare/transforms.py. Example usage, reference, and tests have also been added to demonstrate and validate the new functionality. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @JulioAPeraza - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding benchmarks to assess the performance impact of the new FixedEffectsHedges method, especially for large datasets.
- Explore opportunities for further optimization in the _fit_model method of FixedEffectsHedges, possibly through vectorization of calculations.
- Add more detailed comments in the _fit_model method explaining the mathematical steps, to improve maintainability and verifiability of the implementation.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| se_map = np.zeros(n_voxels, dtype=float) | ||
| tau2_map = np.zeros(n_voxels, dtype=float) | ||
|
|
||
| z_map, p_map, est_map, se_map, tau2_map = [ |
There was a problem hiding this comment.
suggestion: Consider using np.zeros with shape parameter for array initialization
While the list comprehension is an improvement over the previous repetitive code, using np.zeros with a shape parameter (e.g., np.zeros((5, n_voxels), dtype=float)) could be more efficient and clearer in intent for numpy users.
| z_map, p_map, est_map, se_map, tau2_map = [ | |
| import numpy as np | |
| z_map, p_map, est_map, se_map, tau2_map = np.zeros((5, n_voxels), dtype=float) |
| @@ -860,3 +860,48 @@ def z_to_t(z_values, dof): | |||
| t_values = np.zeros(z_values.shape) | |||
| t_values[z_values != 0] = t_values_nonzero | |||
| return t_values | |||
|
|
|||
|
|
|||
| def t_to_d(t_values, sample_sizes): | |||
There was a problem hiding this comment.
suggestion: Add input validation and document assumptions for t_to_d and d_to_g functions
Consider adding input validation to check if the input arrays have the same shape and if the sample sizes are positive. Also, it might be worth adding a note about the assumptions these transformations make, such as equal sample sizes across groups for the t_to_d function.
| def t_to_d(t_values, sample_sizes): | |
| def t_to_d(t_values, sample_sizes): | |
| if not isinstance(t_values, (list, np.ndarray)) or not isinstance(sample_sizes, (list, np.ndarray)): | |
| raise TypeError("t_values and sample_sizes must be lists or numpy arrays.") | |
| if len(t_values) != len(sample_sizes): | |
| raise ValueError("t_values and sample_sizes must have the same length.") | |
| if any(s <= 0 for s in sample_sizes): | |
| raise ValueError("All sample sizes must be positive.") |
| pytest.param( | ||
| ibma.FixedEffectsHedges, | ||
| {"tau2": 0}, | ||
| None, | ||
| {}, | ||
| ("z", "p", "est", "se", "dof"), | ||
| id="FixedEffectsHedges", |
There was a problem hiding this comment.
suggestion (testing): Add edge case tests for FixedEffectsHedges
Consider adding tests for edge cases such as when t_maps contain NaN values, when sample_sizes are zero or negative, and when tau2 is set to a non-zero value. This will ensure the robustness of the FixedEffectsHedges implementation.
| ############################################################################### | ||
| # Fixed Effects Meta-Analysis with Hedges’ g | ||
| # ----------------------------------------------------------------------------- | ||
| from nimare.meta.ibma import FixedEffectsHedges |
There was a problem hiding this comment.
issue (complexity): Consider refactoring to reduce redundancy and improve code organization.
The new code introduces unnecessary complexity and redundancy, violating the DRY (Don't Repeat Yourself) principle. The same block of code is repeated twice, making it harder to maintain. Additionally, the new functionality (Fixed Effects Meta-Analysis with Hedges’ g) is mixed with the original code, disrupting the flow and making it harder to read.
Here's a refactored version that addresses these issues:
from nimare.meta.ibma import FixedEffectsHedges
def plot_results(results):
plot_stat_map(
results.get_map("z"),
cut_coords=[0, 0, -8],
draw_cross=False,
cmap="RdBu_r",
symmetric_cbar=True,
)
print("Description:")
pprint(results.description_)
print("References:")
pprint(results.bibtex_)
# Original functionality
plot_results(results)
# Fixed Effects Meta-Analysis with Hedges’ g
meta = FixedEffectsHedges(tau2=0)
hedges_results = meta.fit(dset)
plot_results(hedges_results)This version reduces redundancy, maintains separation of concerns, and improves code organization, making it easier to understand and maintain.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #894 +/- ##
==========================================
+ Coverage 88.20% 88.23% +0.03%
==========================================
Files 48 48
Lines 6352 6386 +34
==========================================
+ Hits 5603 5635 +32
- Misses 749 751 +2 ☔ View full report in Codecov by Sentry. |
| # tau2 is a float, not a map, so it can't go into the results dictionary | ||
| tables = {"level-estimator": pd.DataFrame(columns=["tau2"], data=[self.tau2])} |
There was a problem hiding this comment.
this is fine for now since it's how weightedleastsquares returns results, but it is awkward to return a single value in a pandas dataframe versus it just being an attribute of the object.
|
@nicholst: If you have time, we have a couple items we could use an extra pair of eyes to affirm the equations were implemented correctly:
|
Closes None.
Changes proposed in this pull request:
Fixed Effects Meta-Analysis with Hedges’ g
Summary by Sourcery
This pull request adds a new feature for performing Fixed Effects Meta-Analysis using Hedges' g, refactors some array initializations for clarity, updates documentation to include the new feature, and adds corresponding test cases.