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.

Sign up for GitHub

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

[REF] Reorganize selcomps and fitmodels_direct #266

Merged
merged 19 commits into from
May 23, 2019
Merged

[REF] Reorganize selcomps and fitmodels_direct #266

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
da0ec2e
Reorganize selcomps and fitmodels_direct
tsalo Apr 21, 2019
4902be8
Fix test.
tsalo Apr 21, 2019
1f9f8c0
[skip-ci] Update docs
tsalo Apr 21, 2019
f55a1e9
Add new tests.
tsalo Apr 22, 2019
e3e307c
Merge branch 'master' into reorg-metrics
tsalo Apr 24, 2019
abbf504
Update value error.
tsalo Apr 25, 2019
ed85e88
Fix test.
tsalo Apr 25, 2019
7e63aa9
Seriously?
tsalo Apr 25, 2019
9dd0e4c
Fix.
tsalo Apr 25, 2019
333a9c5
Merge remote-tracking branch 'ME-ICA/master' into reorg-metrics
tsalo Apr 25, 2019
cf15584
Merge branch 'master' into reorg-metrics
tsalo Apr 25, 2019
f40249f
Fix init file.
tsalo Apr 25, 2019
8378849
More fixing.
tsalo Apr 25, 2019
79ddf59
Merge remote-tracking branch 'ME-ICA/master' into reorg-metrics
tsalo Apr 27, 2019
77255ab
Update docs/api.rst
emdupre Apr 30, 2019
af57118
Revert API name change.
tsalo Apr 30, 2019
e6da8e8
Method -> algorithm
tsalo May 21, 2019
8e4df41
Address review.
tsalo May 23, 2019
540d302
Maybe importing the functions we use would be good.
tsalo May 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 41 additions & 37 deletions docs/api.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,37 @@ API
.. _calibration_ref:


:mod:`tedana.model`: Modeling TE-dependence
--------------------------------------------------
:mod:`tedana.decay`: Modeling signal decay across echoes
tsalo marked this conversation as resolved.
Show resolved Hide resolved
--------------------------------------------------------

.. automodule:: tedana.model
.. automodule:: tedana.decay
:no-members:
:no-inherited-members:

.. autosummary:: tedana.model
:toctree: generated/
.. autosummary:: tedana.decay
:template: function.rst
:toctree: generated/

tedana.model.fitmodels_direct
tedana.decay.fit_decay
tedana.decay.fit_decay_ts

:template: module.rst
.. currentmodule:: tedana

.. _calibration_ref:

tedana.model.fit

:mod:`tedana.combine`: Combining time series across echoes
Copy link
Member

Choose a reason for hiding this comment

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

Here you have combine, but below you've replaced it with model. We should harmonize.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just reordered them based on when they occur in the workflow. It's possible that I missed some changes though, so I apologize if that's adding to the confusion.

----------------------------------------------------------

.. automodule:: tedana.combine
:no-members:
:no-inherited-members:

.. autosummary:: tedana.combine
:toctree: generated/
:template: function.rst

tedana.combine.make_optcom

.. currentmodule:: tedana

Expand Down Expand Up @@ -65,67 +80,65 @@ API
.. _calibration_ref:


:mod:`tedana.combine`: Combine time series
--------------------------------------------------
:mod:`tedana.model`: Computing TE-dependence metrics
tsalo marked this conversation as resolved.
Show resolved Hide resolved
----------------------------------------------------

.. automodule:: tedana.combine
.. automodule:: tedana.model
:no-members:
:no-inherited-members:

.. autosummary:: tedana.combine
.. autosummary:: tedana.model
:toctree: generated/
:template: function.rst

tedana.combine.make_optcom
tedana.model.dependence_metrics
tedana.model.kundu_metrics

:template: module.rst

tedana.combine
tedana.model.fit
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that we want to call this a .fit, since it could be easily confused with the sklearn convention.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've never liked fit either. We can change tedana.model to tedana.metrics (as you suggested above), but we'll still need a file below that in order to keep model/metrics as a subfolder-organized submodule. I fully expect us to expand our sets of metrics (e.g., with AROMA-like motion- and artifact-based ones), which is why I don't think we should just put it in one top-level file (i.e., tedana/metrics.py), but that still leaves us with the issue of what to name the file.

Copy link
Member

Choose a reason for hiding this comment

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

Since we're not quite there yet (e.g., adding motion based metrics), though, I think we should just keep it in a top-level file for now -- we can expand when we add more. WDYT ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, #273 moves the two remaining functions (get_coeffs and computefeats2) in model.fit into a new stats module, so the only things in model.fit will be the metrics (dependence_metrics and kundu_metrics).

Copy link
Member Author

Choose a reason for hiding this comment

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

I can move it there then. So, tedana/model/fit.py will become tedana/model.py and the tedana/model/ folder will be removed. Does that sound good?


.. currentmodule:: tedana

.. _calibration_ref:


:mod:`tedana.decay`: Signal decay
:mod:`tedana.selection`: Component selection
--------------------------------------------------

.. automodule:: tedana.decay
.. automodule:: tedana.selection
:no-members:
:no-inherited-members:

.. autosummary:: tedana.decay
.. autosummary:: tedana.selection
:toctree: generated/
:template: function.rst

tedana.decay.fit_decay
tedana.decay.fit_decay_ts
tedana.selection.manual_selection
tedana.selection.kundu_selection_v2

:template: module.rst

tedana.decay
tedana.selection._utils

.. currentmodule:: tedana

.. _calibration_ref:


:mod:`tedana.selection`: Component selection
:mod:`tedana.gscontrol`: Global signal control
--------------------------------------------------

.. automodule:: tedana.selection
.. automodule:: tedana.gscontrol
:no-members:
:no-inherited-members:

.. autosummary:: tedana.selection
.. autosummary:: tedana.gscontrol
:toctree: generated/
:template: function.rst

tedana.selection.selcomps

:template: module.rst

tedana.selection._utils
tedana.gscontrol.gscontrol_raw
tedana.gscontrol.gscontrol_mmix

.. currentmodule:: tedana

Expand All @@ -146,18 +159,13 @@ API

tedana.io.split_ts
tedana.io.filewrite
tedana.io.gscontrol_mmix
tedana.io.load_data
tedana.io.new_nii_like
tedana.io.write_split_ts
tedana.io.writefeats
tedana.io.writeresults
tedana.io.writeresults_echoes

:template: module.rst

tedana.io

.. currentmodule:: tedana

.. _calibration_ref:
Expand All @@ -182,10 +190,6 @@ API
tedana.utils.make_adaptive_mask
tedana.utils.unmask

:template: module.rst

tedana.utils

.. currentmodule:: tedana

.. _calibration_ref:
7 changes: 4 additions & 3 deletions tedana/decomposition/pca.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,10 +200,11 @@ def tedpca(data_cat, data_oc, combmode, mask, t2s, t2sG,

# Normalize each component's time series
vTmixN = stats.zscore(comp_ts, axis=0)
_, comptable, _, _ = model.fitmodels_direct(
data_cat, comp_ts, eimum, t2s, t2sG, tes, combmode, ref_img,
reindex=False, mmixN=vTmixN, full_sel=False,
comptable, _, _, _ = model.dependence_metrics(
data_cat, data_oc, comp_ts, eimum, t2s, tes, ref_img,
reindex=False, mmixN=vTmixN, method=None,
Copy link
Member

Choose a reason for hiding this comment

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

method is a little too ambiguous of an argument, here.

label='mepca_', out_dir=out_dir, verbose=verbose)

# varex_norm from PCA retained on top of varex from fitmodels_direct
comptable['original normalized variance explained'] = varex_norm

Expand Down
4 changes: 2 additions & 2 deletions tedana/model/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
# ex: set sts=4 ts=4 sw=4 et:

from .fit import (
fitmodels_direct, get_coeffs, computefeats2
dependence_metrics, kundu_metrics, get_coeffs, computefeats2
)

__all__ = [
'fitmodels_direct', 'get_coeffs', 'computefeats2']
'dependence_metrics', 'kundu_metrics', 'get_coeffs', 'computefeats2']
Loading