Do not zero out one-tailed z-statistics for p-values > 0.5 #693

JulioAPeraza · 2022-06-01T21:39:25Z

Closes #511.

Changes proposed in this pull request:

Do not zero out one-tailed z-statistics for p-values > 0.5. Note: p-value = 0.5 will still be set to 0, so logp and z-statistic maps will not completely match in that case.

codecov · 2022-06-01T22:47:32Z

Codecov Report

Base: 88.64% // Head: 88.66% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (e6cb5e2) compared to base (2de404d).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #693      +/-   ##
==========================================
+ Coverage   88.64%   88.66%   +0.02%     
==========================================
  Files          38       38              
  Lines        4324     4323       -1     
==========================================
  Hits         3833     3833              
+ Misses        491      490       -1

Impacted Files	Coverage Δ
nimare/decode/continuous.py	`94.79% <ø> (ø)`
nimare/io.py	`95.14% <ø> (ø)`
nimare/meta/cbma/ale.py	`96.61% <ø> (ø)`
nimare/meta/cbma/mkda.py	`98.75% <ø> (ø)`
nimare/meta/kernel.py	`95.78% <ø> (ø)`
nimare/meta/utils.py	`95.73% <ø> (ø)`
nimare/extract/utils.py	`38.58% <100.00%> (ø)`
nimare/transforms.py	`75.63% <100.00%> (+0.27%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

tsalo

The changes make sense to me. Can you add a new test that covers this case though (or expand an existing one)?

nimare/transforms.py

JulioAPeraza · 2022-06-07T19:21:37Z

@tsalo if we don't zero out one-tailed z-statistics for p-values > 0.5, should we: (1) leave the negative value, (2) apply the abs() function since the values returned by this function should be unsigned z-statistics, or (3) replace the negative values with a small number 1.0e-300?

tsalo · 2022-06-11T14:34:00Z

I'm not sure... maybe someone else who has a better handle on stats can answer that?

tsalo · 2023-01-27T16:41:05Z

@yifan0330 seemed to have a good handle on z-to-p conversion (and probably p-to-z conversion), given #748 and #749. @yifan0330 would you be willing to take a look at this?

yifan0330 · 2023-01-31T22:57:56Z

nimare/tests/test_transforms.py

    [
        (0.0, "two", 1.0),
        (0.0, "one", 0.5),
        (1.959963, "two", 0.05),
        (1.959963, "one", 0.025),
        (-1.959963, "two", 0.05),
+        (-1.959963, "one", 0.025),


This test case should be (-1.959964, "one", 0.0975). I think I've corrected in issue #748

yifan0330 · 2023-01-31T22:59:30Z

nimare/tests/test_transforms.py

        ([0.0, 1.959963, -1.959963], "two", [1.0, 0.05, 0.05]),
+        ([0.0, 1.959963, -1.959963], "one", [0.5, 0.025, 0.025]),


([0.0, 1.959963, -1.959963], "one", [0, 0.025, 0.975])

yifan0330 · 2023-01-31T23:07:12Z

@yifan0330 seemed to have a good handle on z-to-p conversion (and probably p-to-z conversion), given #748 and #749. @yifan0330 would you be willing to take a look at this?

@JulioAPeraza @tsalo I saw you replace negative z value with values estimated by cdf, I am wondering if it's the same as taking absolute value of z (since z and zval_cdf have same value but opposite sign)? Also, I left two comments on testing case (I think z=-1.959964 corresponds to p=0.975 in one tail test).

JulioAPeraza · 2023-02-01T18:06:49Z

Thanks so much, @yifan0330! I've incorporated your suggestions.
@tsalo Do you think this is ready to merge?

tsalo

@JulioAPeraza if @yifan0330 is happy with the changes then absolutely, I think it's good to merge.

yifan0330 · 2023-02-01T21:10:48Z

@JulioAPeraza if @yifan0330 is happy with the changes then absolutely, I think it's good to merge.

Yeah I am happy with Julio's changes :)

JulioAPeraza · 2023-02-01T21:15:38Z

Thanks! Merging now.

)" This reverts commit 87964b8.

…769) * Revert "Do not zero out one-tailed z-statistics for p-values > 0.5 (#693)" This reverts commit 87964b8. * Solve `black` issues * Add a note about why we zero out negative z-scores

@yifan0330

…f#693) * Do not zero out one-tailed z-statistics for p-values > 0.5 * add comment * Replace negative values in z with values estimated by CDF * Add test for p to z conversion * @yifan0330 Apply suggestions from code review * Random tests failing. Run black

…eurostuff#769) * Revert "Do not zero out one-tailed z-statistics for p-values > 0.5 (neurostuff#693)" This reverts commit 87964b8. * Solve `black` issues * Add a note about why we zero out negative z-scores

@tsalo

* add cbmr.py file * create a design matrix function for cbmr * add test file for cbmr * modify pre-process and training function in cbmr * modify the dataset.anotations in cbmr * add documentation in utils functions * update model structure * update optimizer function * [skip ci][wip] update loss function * update _fit function in CBMR * [wip][skip ci] allow other data types as pre-process inputs * use a sparse array instead of numpy * [skip ci][wip] allow for multiple-group cbmr * [skip ci][wip] fix conflict to merge * [skip CI][wip] modify settings in pre_process * [skip ci][wip] implemented group-wise CBMR and fix problems * [skip ci][wip] add results as inputs to MetaResults * [skip ci][wip] modify standardization of group moderators * [skip ci][wip] implement NB regression * [skip ci][wip]remove vox2idx function and simplify the code * [skip ci][wip]develp CNB model * adjustment to Firth penalty * [skip CI][wip] implement index2voxel function * [skip CI][wip] add implementation for SE of regression coefficient * [skip CI][WIP] implementing CBMRInference * [skip CI][wip] implement spatial homogeneity test * [skip ci][wip] implement CBMRInference group-wise comparison * formalize GLH contrast variable * [skip ci][wip] implemented Cov in GLH for all three models * [skip CI][wip] add a demonstration for CBMREstimator & CBMRInference * [skip CI][wip] modify example files for demonstrating CBMR * add documentation to functions. * solve some issues suggested by flake8 * [skip CI][WIP] fix a bug in log-likelihood function of CNB model * [skip CI][WIP] Update code according to comments * [skip CI][WIP] solve conflicts in code * restructure code * [skip CI][WIP] replace variable name and remove study_level_moderators * [skip CI][WIP] changed variables names to be more intuitive. * reorganize model classes to be partially initialized * [skip CI][WIP] set some params as attribute of CBMREstimator Class. * restruct inference code to models * add some code for overdispersion model class. * change model to use optimizer * change model names * refactor the optimizer functions into the model class * create a fit method for models * add summary to model fit * function name suggestions * make square_root an attribute * allow categorical variables in CBMR * fix a bug * new changes on inference class * solve conflict * restruct code in CBMRInference * add documentation foor create_contrast function * add new steps: remove duplicate rows in contrast matrix * modify documentation and comments * change function name to snake case * restruct code and remove repetition * reconstruct code, remove repeated code * correct testing cases of z_to_p function * add regular expression code to CBMRInference * [skip CI][WIP] update example file based on reconstructed code * [skip CI][WIP] Tried standardized categorical covariates * Raise deprecation warnings with Python 3.6 and 3.7 (#754) * Add deprecation warnings for Python 3.6 and 3.7 * Remove arrays from exclude in Github action * Run tests and minimum dependencies on python 3.8 * Run linting and publish on 3.8 * Ignore D401 warnings * Update setup.cfg * Update testing.yml * [MAINT] Fix various errors due to major version changes in dependencies (#757) * bump matplotlib version to handle new nilearn release * restrict numpy versions due to numba issue * make nibabel less than version 5.0 * Remove "dataset" `return_type` option from kernel transformers (#752) * Remove "dataset" `return_type` option from kernel transformers * Drop tests ma_map_reuse * Update test_meta_kernel.py * Update test_meta_kernel.py * Update nimare/meta/kernel.py Co-authored-by: Taylor Salo <tsalo90@gmail.com> * Update nimare/meta/kernel.py Co-authored-by: Taylor Salo <tsalo90@gmail.com> * Add versionchanged to child classes Co-authored-by: Taylor Salo <tsalo90@gmail.com> * [skip ci] Update CHANGELOG * Support nibabel 5.0.0 (#762) * Add header to `Nifti1Image` when passing an int64 array * @tsalo Apply suggestions from code review * Update test_annotate_gclda.py * Do not zero out one-tailed z-statistics for p-values > 0.5 (#693) * Do not zero out one-tailed z-statistics for p-values > 0.5 * add comment * Replace negative values in z with values estimated by CDF * Add test for p to z conversion * @yifan0330 Apply suggestions from code review * Random tests failing. Run black * Link to NeuroStars software support category instead of neuro questions (#768) * Link to software support category. * Add links to GitHub repo. * Update nilearn URL. * Revert "Do not zero out one-tailed z-statistics for p-values > 0.5" (#769) * Revert "Do not zero out one-tailed z-statistics for p-values > 0.5 (#693)" This reverts commit 87964b8. * Solve `black` issues * Add a note about why we zero out negative z-scores * create a design matrix function for cbmr * [skip CI][WIP] solve conflicts * update model structure * use a sparse array instead of numpy * [skip CI][WIP] solve conflict * solve conflicts. * [skip ci][wip] modify standardization of group moderators * [skip CI][wip] implement index2voxel function * [skip CI][wip] add implementation for SE of regression coefficient * [skip CI][wip] add a demonstration for CBMREstimator & CBMRInference * [skip CI][WIP] fix a bug in log-likelihood function of CNB model * [skip CI][WIP] Update code according to comments * refactor the optimizer functions into the model class * create a fit method for models * allow categorical variables in CBMR * restruct code in CBMRInference * [skip CI][WIP] update example file based on reconstructed code * solve conflict * [skip CI][WIP] solve conflicts * solve conflicts * [skip CI][WIP] work on example file * [skip CI][WIP] complete example file for cbmr. * [skip CI][WIP] implement an option to specify the reference subtype for categorical moderators. * [skip CI][WIP] rewrite cbmr example in py file. * [skip CI][WIP] modify corrector class to be consistent with cbmr outputs * [skip CI][WIP] add FDR/FWE correction methods to test * add testing cases with more coverage for CBMREstimator * [skip CI] [WIP] added new changes * run black and isort * wip: working through refactor * more refactor * remove debug info * fix errors * test firth penalty * black formating * more formatting * remove peaks2maps * remove redundant def * change documentation line * move patsy into function * add necessary installs * update example notebook with api * increase spacing and tolerance * fix estimator name * sync utils with main * update to main on z_to_p test * remove conperm workflow * remove whitespace * fix some errors * make explicit where to document * make StandardizeField a transformer * add functorch for python 3.6 * try to use older version of functorch * loosen restriction * fix bugs in cbmr example file * [skip CI][WIP] fix bugs in testing function for cbmr_update * add documentation for models.py * add documentation for cbmr.py * add documentation for utils.py * add description for CBMREstimator * change lr to a smaller value * edit description function and add reference. * check if result.__description is a string. * resolve merge conflict * set random seed * simplify the log-likelihood function of NB model * simplify the log-likelihood function of NB model * implement wald test for CBMRInference * edit testing function for cbmr * edit testing function for correctors * fix linting error * fix linting error * fix linting error * fixed linting error * fix linting error * fix linting error * fix linting error * fix linting error * fix linting error * fix linting error * remove unused test datasets * fix linter error * add cbmr to docs/api.rst * edit example file for cbmr * remove the standardize_field function as it's replicated in the StandardizeField class * use pass instead of return in the abstract methods * added a test for StandardizeField class. * edit example file of cbmr methods. * fix linter error. * fix a linter error. * fix a linter error * fix names of notebooks * remove functorch (it was absorbed into torch) --------- Co-authored-by: Yifan Yu <pra123@rescomp1.hpc.in.bmrc.ox.ac.uk> Co-authored-by: Yifan Yu <pra123@compg007.hpc.in.bmrc.ox.ac.uk> Co-authored-by: Yifan Yu <pra123@compg005.hpc.in.bmrc.ox.ac.uk> Co-authored-by: James Kent <jamesdkent21@gmail.com> Co-authored-by: Yifan Yu <pra123@compg006.hpc.in.bmrc.ox.ac.uk> Co-authored-by: Julio A. Peraza <52050407+JulioAPeraza@users.noreply.github.com> Co-authored-by: Taylor Salo <tsalo90@gmail.com> Co-authored-by: jdkent <jdkent@users.noreply.github.com> Co-authored-by: Taylor Salo <salot@pennmedicine.upenn.edu>

Do not zero out one-tailed z-statistics for p-values > 0.5

d6d6c9b

JulioAPeraza added the bug Issues noting problems and PRs fixing those problems. label Jun 1, 2022

tsalo requested changes Jun 2, 2022

View reviewed changes

nimare/transforms.py Show resolved Hide resolved

JulioAPeraza added 3 commits June 2, 2022 10:22

add comment

432425f

Replace negative values in z with values estimated by CDF

f60745b

Add test for p to z conversion

34d14b1

JulioAPeraza added the help wanted Extra attention is needed label Jan 27, 2023

yifan0330 reviewed Jan 31, 2023

View reviewed changes

JulioAPeraza and others added 3 commits February 1, 2023 10:46

Merge branch 'neurostuff:main' into p2z-transform

24fda70

@yifan0330 Apply suggestions from code review

760d248

Random tests failing. Run black

e6cb5e2

tsalo approved these changes Feb 1, 2023

View reviewed changes

JulioAPeraza merged commit 87964b8 into neurostuff:main Feb 1, 2023

JulioAPeraza deleted the p2z-transform branch February 1, 2023 21:15

JulioAPeraza restored the p2z-transform branch February 2, 2023 23:32

JulioAPeraza added a commit that referenced this pull request Feb 2, 2023

Revert "Do not zero out one-tailed z-statistics for p-values > 0.5 (#693

5efc54c

)" This reverts commit 87964b8.

JulioAPeraza mentioned this pull request Feb 4, 2023

Revert "Do not zero out one-tailed z-statistics for p-values > 0.5" #769

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not zero out one-tailed z-statistics for p-values > 0.5 #693

Do not zero out one-tailed z-statistics for p-values > 0.5 #693

JulioAPeraza commented Jun 1, 2022

codecov bot commented Jun 1, 2022 •

edited

tsalo left a comment

JulioAPeraza commented Jun 7, 2022

tsalo commented Jun 11, 2022

tsalo commented Jan 27, 2023

yifan0330 Jan 31, 2023

yifan0330 Jan 31, 2023

yifan0330 commented Jan 31, 2023

JulioAPeraza commented Feb 1, 2023

tsalo left a comment

yifan0330 commented Feb 1, 2023

JulioAPeraza commented Feb 1, 2023

		([0.0, 1.959963, -1.959963], "two", [1.0, 0.05, 0.05]),
		([0.0, 1.959963, -1.959963], "one", [0.5, 0.025, 0.025]),

Do not zero out one-tailed z-statistics for p-values > 0.5 #693

Do not zero out one-tailed z-statistics for p-values > 0.5 #693

Conversation

JulioAPeraza commented Jun 1, 2022

codecov bot commented Jun 1, 2022 • edited

Codecov Report

tsalo left a comment

Choose a reason for hiding this comment

JulioAPeraza commented Jun 7, 2022

tsalo commented Jun 11, 2022

tsalo commented Jan 27, 2023

yifan0330 Jan 31, 2023

Choose a reason for hiding this comment

yifan0330 Jan 31, 2023

Choose a reason for hiding this comment

yifan0330 commented Jan 31, 2023

JulioAPeraza commented Feb 1, 2023

tsalo left a comment

Choose a reason for hiding this comment

yifan0330 commented Feb 1, 2023

JulioAPeraza commented Feb 1, 2023

codecov bot commented Jun 1, 2022 •

edited