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

Generate metrics from external regressors using F stats #1064

Open
wants to merge 76 commits into
base: main
Choose a base branch
from

Conversation

handwerkerd
Copy link
Member

@handwerkerd handwerkerd commented Mar 20, 2024

Closes #1009. This is an alternative approach to #1008 and #1021.

If a user provides external regressors, this will calculate a fit to those regressors to include as metrics in the component table and for use in decision trees.

Changes proposed in this pull request:

  • Create a new module, metrics.external, for calculating metrics from external regressors
    • An F statistic is used with a polynomial detrending baseline and p, F, and R2 values are saved as metrics in the component table
    • These stats are saved for the Full F model using all regressors as well as partial F models. This makes it possible to both examine when the full model fits, but to also log if it's the motion vs ROI-based vs cardiac/respiration-based regressors that fit to a specific component time series.
    • There is a special class of regressors called task_keep If regressors under this label are included, these will be excluded from the full F model and a separate full F model will be calculated using just these regressors. This was a suggestion from @dowdlelt so that it would be possible to identify and conservative retain components that fit to the overall task design.
  • Add a new parameter to the tedana CLI, --external, to pass in a TSV with external regressors.
  • Draft decision trees that incorporate external regressor correlation metrics.
    • In the decision tree there is a new field external_regressor_config which is a dictionary with the following keys:
      • info A description of what the config does that's saved
      • report A description to add to report.txt
      • calc_stats Currently the only option is "F" but this leaves open possibilities for additional options.
      • detrend: Will automatically calculate the number of polynomial detrending regressors if true, but can also be a number for the users to specify. If this is false, then will be set to 0 (mean removal) and log a warning.
      • f_states_partial_models [optional] A list of titles for the partial models (i.e. ["Motion", "CSF"])
      • If f_states_partial_models has model names, each model name is its own key and contains either a list of column labels or a regular express wildcard (i.e. "Motion": ["^mot_.*$"] means the Motion partial model will include any external regressor column label that begins with mot_ and "Motion": ["mot_x", "mot_y", "mot_z"] specifies 3 specific column label names
      • task_keep [optional] Contents are a regex wildcard or specific names to define task regressors that will not be included in the full model
    • demo_minimal_external_regressors_motion_task_models.json uses all the above options, uses the partial models to add a classification_tag, but not change results and retains components that correlate to the task (R2>0.5), have kappa>elbow irregardless of what rho is.
    • demo_minimal_external_regressors_single_model.json uses the minimum number of options to run with external regressors.
  • Includes tests that should cover all new code

To do:

  • Update documentation to explain all this new functionality and outputs (waiting on this for enough of us to agree that the approach is generally good and we're not making more substantive changes to the structure/parameters/etc

Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through and added comments. The method looks solid to me. I have a few thoughts on style though:

  1. There's a lot of commented vestigial code.
  2. The docstrings should have type annotations, per Incorporate type hints when possible #704. I would also recommend requiring parameter named in the function calls (i.e., by putting a leading * before the parameters).
  3. Some variable names do not match the rest of the codebase (e.g., you use n_time instead of n_vols).
  4. It would be great if strings and lines in docstrings were broken on punctuation. This will result in cleaner diffs in the future.
  5. We're going to need extensive tests to cover the new code.

tedana/metrics/_utils.py Outdated Show resolved Hide resolved
tedana/metrics/_utils.py Outdated Show resolved Hide resolved
tedana/metrics/collect.py Outdated Show resolved Hide resolved
tedana/metrics/collect.py Outdated Show resolved Hide resolved
tedana/metrics/external.py Outdated Show resolved Hide resolved
tedana/metrics/_utils.py Outdated Show resolved Hide resolved
tedana/metrics/external.py Outdated Show resolved Hide resolved
tedana/metrics/external.py Outdated Show resolved Hide resolved
tedana/metrics/external.py Outdated Show resolved Hide resolved
tedana/metrics/external.py Outdated Show resolved Hide resolved
handwerkerd and others added 9 commits May 2, 2024 15:29
Co-authored-by: Taylor Salo <salot@pennmedicine.upenn.edu>
Co-authored-by: Taylor Salo <salot@pennmedicine.upenn.edu>
Co-authored-by: Taylor Salo <salot@pennmedicine.upenn.edu>
Co-authored-by: Taylor Salo <salot@pennmedicine.upenn.edu>
Co-authored-by: Taylor Salo <salot@pennmedicine.upenn.edu>
@handwerkerd
Copy link
Member Author

handwerkerd commented May 2, 2024

@tsalo I just added type hints to external.py I'm still a bit of a novice with type hints and it looks like formatting is non-trivially changing across python versions. I tried to follow guidance that's valid for python 3.8 since that's our oldest permitted version. I'll keep working in other files and on your remaining review comments, but let me know if I'm appropriately adding type hints so far.

Update as I was writing this comment: Lots of tests just failed so the answer to that question is"no". It looks numpy v1.24.3 that's currently in my python 3.8 environment includes numpy.typing and not numpy._typing while numpy v1.26.3 in my python 3.11 environment includes numpy._typing and not numpy.typing. I suspect there's a way to set up backwards compatibility for this, but I wanted to share what I'm seeing before I switch to childcare stuff. I figured out the issue and all tests are passing now. It seems like using import numpy.typing as npt works, but directly referencing np.typing does not work across all versions

@handwerkerd
Copy link
Member Author

@tsalo Can you explain more what you mean by "It would be great if strings and lines in docstrings were broken on punctuation. This will result in cleaner diffs in the future."

@tsalo
Copy link
Member

tsalo commented May 3, 2024

Absolutely. Take a paragraph in the text, like the one below:

This is sentence one. This is
sentence two. This is sentence
three- but it's still going.

Since sentences are a unit within the paragraph, we're more likely to change them than random words.
Plus, since almost everything we write is in markdown or rst, the newlines don't impact the rendered text- just the code.

-This is sentence one. This is
+This is sentence one. This is a 
-sentence two. This is sentence
+new sentence two with other
-three- but it's still going.
+changes. This is sentence three-
+but it's still going.

The diff on that, where the only consideration is line length, is much more extensive (and harder to interpret for a reviewer) than the diff if we broke up the text on punctuation:

 This is sentence one.
-This is sentence two.
+This is a new sentence 
+two with other changes.
 This is sentence three-
 but it's still going.

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 option to classify components based on their correlation with regressors
2 participants