-
Notifications
You must be signed in to change notification settings - Fork 97
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
Adding a degree of freedom (dof) flag (--n-independent-echos) to allow users to set the dof used in fstat calculations #1177
base: main
Are you sure you want to change the base?
Changes from 17 commits
554faa9
e940df1
05acdc9
2673ea2
bd63bf7
eb83997
cfde9a4
6551f05
731737a
7a5df43
c209c6a
b57dbb7
eb4691a
53c46d0
ab07715
deb431a
a126d7f
a5964cf
6b846cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,3 +123,4 @@ ENV/ | |
|
||
# vim swap files | ||
*.swp | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -714,10 +714,19 @@ def calc_kappa_elbow( | |
This also means the kappa elbow should be calculated before those two other functions | ||
are called | ||
""" | ||
if ( | ||
"echo_dof" in selector.cross_component_metrics_.keys() | ||
and selector.cross_component_metrics_["echo_dof"] | ||
): | ||
echo_dof = selector.cross_component_metrics_["echo_dof"] | ||
else: | ||
# DOF is number of echoes if not otherwise specified | ||
echo_dof = selector.cross_component_metrics_["n_echos"] | ||
Comment on lines
+723
to
+724
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to clarify- DOF wouldn't be the number of echoes, right? It would be the number of echoes minus 1. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ug. I'm fairly sure in all calculations that use echo_dof, it's I think a terminology-consistent solution should be to make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You comment below of changing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm yeah I'm not sure. Im going to check with Tom on this and Ill get back to you. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do think @tsalo is correct here. My proposal is:
This would mean that the user facing parameter has a clearer meaning that more accurately matches what we're currently doing in the code. This might mess up existing scripts that Katie & Charles are using, but hopefully that won't be a huge problem. @katielamar I know you're preparing your talk right now. Tell me if you want me to make this change & open another PR on your branch for you to review. |
||
outputs = { | ||
"decision_node_idx": selector.current_node_idx_, | ||
"node_label": None, | ||
"n_echos": selector.cross_component_metrics_["n_echos"], | ||
"echo_dof": echo_dof, | ||
"used_metrics": {"kappa"}, | ||
"calc_cross_comp_metrics": [ | ||
"kappa_elbow_kundu", | ||
|
@@ -777,7 +786,7 @@ def calc_kappa_elbow( | |
outputs["varex_upper_p"], | ||
) = kappa_elbow_kundu( | ||
selector.component_table_, | ||
selector.cross_component_metrics_["n_echos"], | ||
echo_dof, | ||
comps2use=comps2use, | ||
) | ||
selector.cross_component_metrics_["kappa_elbow_kundu"] = outputs["kappa_elbow_kundu"] | ||
|
@@ -846,10 +855,20 @@ def calc_rho_elbow( | |
f"It is {rho_elbow_type} " | ||
) | ||
|
||
if ( | ||
"echo_dof" in selector.cross_component_metrics_.keys() | ||
and selector.cross_component_metrics_["echo_dof"] | ||
): | ||
echo_dof = selector.cross_component_metrics_["echo_dof"] | ||
else: | ||
# DOF is number of echoes if not otherwise specified | ||
echo_dof = selector.cross_component_metrics_["n_echos"] | ||
|
||
outputs = { | ||
"decision_node_idx": selector.current_node_idx_, | ||
"node_label": None, | ||
"n_echos": selector.cross_component_metrics_["n_echos"], | ||
"echo_dof": echo_dof, | ||
"calc_cross_comp_metrics": [ | ||
elbow_name, | ||
"rho_allcomps_elbow", | ||
|
@@ -904,7 +923,7 @@ def calc_rho_elbow( | |
outputs["elbow_f05"], | ||
) = rho_elbow_kundu_liberal( | ||
selector.component_table_, | ||
selector.cross_component_metrics_["n_echos"], | ||
echo_dof, | ||
rho_elbow_type=rho_elbow_type, | ||
comps2use=comps2use, | ||
subset_comps2use=subset_comps2use, | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -15,7 +15,7 @@ | |||||||||||||
F_MAX = 500 | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
def kundu_tedpca(component_table, n_echos, kdaw=10.0, rdaw=1.0, stabilize=False): | ||||||||||||||
def kundu_tedpca(component_table, n_echos, echo_dof=None, kdaw=10.0, rdaw=1.0, stabilize=False): | ||||||||||||||
"""Select PCA components using Kundu's decision tree approach. | ||||||||||||||
|
||||||||||||||
Parameters | ||||||||||||||
|
@@ -25,6 +25,10 @@ def kundu_tedpca(component_table, n_echos, kdaw=10.0, rdaw=1.0, stabilize=False) | |||||||||||||
variance explained. Component number should be the index. | ||||||||||||||
n_echos : :obj:`int` | ||||||||||||||
Number of echoes in dataset. | ||||||||||||||
echo_dof : int | ||||||||||||||
Degree of freedom to use in goodness of fit metrics (fstat). | ||||||||||||||
Primarily used for EPTI acquisitions. | ||||||||||||||
If None, number of echoes will be used. Default is None. | ||||||||||||||
kdaw : :obj:`float`, optional | ||||||||||||||
Kappa dimensionality augmentation weight. Must be a non-negative float, | ||||||||||||||
or -1 (a special value). Default is 10. | ||||||||||||||
|
@@ -59,8 +63,11 @@ def kundu_tedpca(component_table, n_echos, kdaw=10.0, rdaw=1.0, stabilize=False) | |||||||||||||
+ 1 | ||||||||||||||
] | ||||||||||||||
varex_norm_cum = np.cumsum(component_table["normalized variance explained"]) | ||||||||||||||
if echo_dof is None: | ||||||||||||||
fmin, fmid, fmax = getfbounds(n_echos) | ||||||||||||||
else: | ||||||||||||||
fmin, fmid, fmax = getfbounds(echo_dof) | ||||||||||||||
Comment on lines
+66
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
|
||||||||||||||
fmin, fmid, fmax = getfbounds(n_echos) | ||||||||||||||
if int(kdaw) == -1: | ||||||||||||||
lim_idx = ( | ||||||||||||||
utils.andb([component_table["kappa"] < fmid, component_table["kappa"] > fmin]) == 2 | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,24 +11,26 @@ | |
RepLGR = logging.getLogger("REPORT") | ||
|
||
|
||
def getfbounds(n_echos): | ||
def getfbounds(echo_dof): | ||
""" | ||
Get F-statistic boundaries based on number of echos. | ||
|
||
Parameters | ||
---------- | ||
n_echos : :obj:`int` | ||
Number of echoes | ||
echo_dof : :obj:`int` | ||
Degree of freedom to use in goodness of fit metrics (fstat). | ||
Typically the number of echos in the multi-echo data | ||
May be a lower value for EPTI acquisitions. | ||
|
||
Returns | ||
------- | ||
fmin, fmid, fmax : :obj:`float` | ||
F-statistic thresholds for alphas of 0.05, 0.025, and 0.01, | ||
respectively. | ||
""" | ||
f05 = stats.f.ppf(q=(1 - 0.05), dfn=1, dfd=(n_echos - 1)) | ||
f025 = stats.f.ppf(q=(1 - 0.025), dfn=1, dfd=(n_echos - 1)) | ||
f01 = stats.f.ppf(q=(1 - 0.01), dfn=1, dfd=(n_echos - 1)) | ||
f05 = stats.f.ppf(q=(1 - 0.05), dfn=1, dfd=(echo_dof - 1)) | ||
f025 = stats.f.ppf(q=(1 - 0.025), dfn=1, dfd=(echo_dof - 1)) | ||
f01 = stats.f.ppf(q=(1 - 0.01), dfn=1, dfd=(echo_dof - 1)) | ||
Comment on lines
+31
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's misleading to call the parameter dof and then subtract 1, unless I'm misunderstanding the math. We should change the function to use the degrees of freedom and pass in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or just change the |
||
return f05, f025, f01 | ||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be simpler to just set
echo_dof
aslen(tes)
if it's not provided, at the top of this function.However, I am concerned that setting the degrees of freedom based on the number of echoes is already a bug (see #811). It just doesn't account for the fact that the degrees of freedom varies across voxels due to the adaptive mask.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree on this change.
Addressing #811 might be useful, but seems beyond the scope of this PR. If we're clarifying which inputs are supposed to be DOF, this might be easier to address.