Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENH] Support Dataset transformations in kernel transformers #320

Merged
merged 18 commits into from
Aug 29, 2020

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Aug 27, 2020

Closes #41 and closes #307 and references #195.

Changes proposed in this pull request:

  • Add copy method to Dataset so that kernel transformers, when returning Datasets, don't update the original object.
  • Track image basepath in Dataset.
  • Support Dataset transformation in kernel transformers. This transformation creates an unmasked version of the MA map, saves it to a file based on Dataset.basepath and the parameters of the transformer, and stores the path in the Dataset.images attribute.
  • Fix bug in nimare.stats.null_to_p(). Values outside the range of the null distribution (i.e., p = 0) are now cropped down to epsilon for float.

To do:

  • Do not mask MA maps when saving to files. This way they're template-, but not mask-, specific.
  • Load images from files when possible. Use masker to apply the mask.
  • Move as much as possible to the base class.
  • Drop absolute paths from images DataFrame. The new basepath attribute should be sufficient.
  • Update tests.

Something to perhaps keep an eye on is that there was a new overflow error in one of the MKDA tests. Since these changes shouldn't impact behavior, I think the new error (which reflects a bug in null_to_p), probably stems from a random seed thing.

tsalo added 3 commits August 27, 2020 15:12
- Don't mask MA maps when return_type is dataset.
- Try to load MA maps from dataset before generating them.
@codecov
Copy link

codecov bot commented Aug 29, 2020

Codecov Report

Merging #320 into master will decrease coverage by 0.03%.
The diff coverage is 74.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #320      +/-   ##
==========================================
- Coverage   77.18%   77.15%   -0.04%     
==========================================
  Files          38       38              
  Lines        3678     3817     +139     
==========================================
+ Hits         2839     2945     +106     
- Misses        839      872      +33     
Impacted Files Coverage Δ
nimare/meta/kernel.py 76.30% <72.18%> (-3.20%) ⬇️
nimare/base.py 87.06% <100.00%> (+0.46%) ⬆️
nimare/dataset.py 89.22% <100.00%> (+1.06%) ⬆️
nimare/meta/mkda.py 96.88% <100.00%> (ø)
nimare/stats.py 88.67% <100.00%> (+0.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 530e652...0b5cdb4. Read the comment docs.

@tsalo tsalo marked this pull request as ready for review August 29, 2020 18:38
@tsalo
Copy link
Member Author

tsalo commented Aug 29, 2020

I'm going to hold off on abstracting things to the base class until later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add path attribute to Dataset Have kernel classes return Dataset instances
1 participant