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

Jump to bottom

Adding a degree of freedom (dof) flag (--n-independent-echos) to allow users to set the dof used in fstat calculations #1177

Merged
merged 39 commits into from
Mar 21, 2025
Merged

Adding a degree of freedom (dof) flag (--n-independent-echos) to allow users to set the dof used in fstat calculations #1177

merged 39 commits into from
Mar 21, 2025

Conversation

katielamar
Copy link
Contributor

@katielamar katielamar commented Feb 25, 2025 •
edited
Loading

Hi everyone,
I've updated the tedana code to address issue #1168. I've made this a draft PR since this is my first time contributing to Tedana and I'm a bit new to submitting pull requests. Let me know if you have any suggestions on how I can improve the documentation/code.

Closes #1168.

Summary of Updates:

  • User can now set the degree of freedom that will be used for the fstat calculation. More info: No BOLD components detected error #1168.
  • Tedana from CMD Line: Use --n-independent-echos to set the degrees of freedom when running tedana workflow from the command line
  • Tedana from Python: Set the variable echo_dof when calling the tedana_workflow function

tsalo
tsalo reviewed Feb 25, 2025
View reviewed changes
@katielamar katielamar marked this pull request as ready for review February 25, 2025 18:52
handwerkerd
handwerkerd requested changes Feb 26, 2025
View reviewed changes
Copy link
Member

@handwerkerd handwerkerd left a comment

Choose a reason for hiding this comment

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

Besides the one ongoing discussion about a warning message, the rest of this looks good.

We should definitely include some tests. I can help with those, if useful.

@katielamar
Copy link
Contributor Author

@handwerkerd The tests/changes you made are in the latest commit. I also made a change to fix the fstat map test you added for echo_dof.

assert np.min(f_t2_maps_orig - f_t2_maps) >= 0
assert np.min(f_s0_maps_orig - f_s0_maps) >= 0

One of the asserts is still failing in fstat map test. Its caused by the following failing. Should this test be strictly greater than 0? I think maybe the f_max may be preventing that. Can you take a look?

assert np.min(f_t2_maps_orig[adaptive_mask == 5] - f_t2_maps[adaptive_mask == 5]) > 0
assert np.min(f_s0_maps_orig[adaptive_mask == 5] - f_s0_maps[adaptive_mask == 5]) > 0

@handwerkerd
Copy link
Member

Since the tests were passing on my local computer & failing here, I decided to push some commits to see if I could just solve it. I think the key problem was we have a maximum F value set to 500. That means, for a test >500, the F is a constant 500 and the difference between DOF==3 vs 5 is 0. Since the goal of the test was to make sure the change in DOF always changed the F stats, I needed to mask out the F==500 tests for both f_t2 and f_s0.

@handwerkerd
Copy link
Member

@tsalo Do you want to take another look or is this good to merge?

handwerkerd and others added 11 commits March 13, 2025 09:30
@katielamar
Copy link
Contributor Author

@handwerkerd some of the tests are failing. I merged your PR with my repo and made an update to get one of the unit tests to pass (changed np.concat to np.concatenate) and now a bunch of tests that were once passing are failing. Can you take a look? It seems to be due to an assertion you put in for the adaptive mask test.

handwerkerd
handwerkerd reviewed Mar 14, 2025
View reviewed changes
@handwerkerd
Copy link
Member

@handwerkerd some of the tests are failing. I merged your PR with my repo and made an update to get one of the unit tests to pass (changed np.concat to np.concatenate) and now a bunch of tests that were once passing are failing. Can you take a look? It seems to be due to an assertion you put in for the adaptive mask test.

I think found the issue. A warning message in make_adaptive_mask was automatically reformatted due to tedana using pre-commit & that removed some necessary spaces in the message. There were also some failed tests due to being unable to connect to OSF, but I think that's (again) an internet issue & not a code issue. Besides that, the code passes tests on my computer & it looks like the previously failing tests are now passing here.

@tsalo Anything else to address before merging?

@handwerkerd handwerkerd mentioned this pull request Mar 16, 2025
Merged
tsalo
tsalo reviewed Mar 17, 2025
View reviewed changes
tsalo
tsalo approved these changes Mar 21, 2025
View reviewed changes
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.

The changes look good to me. I have one small suggestion to fix a typo. If you could merge that suggestion before merging this PR, that would be great. No need to request a fresh review after that.

One code comment that I haven't gotten clarification on is the claim that "EPTI has less dropout". Is that accurate or more a factor of having more echoes at shorter echo times? For example, for an echo at 40 ms, will EPTI show less dropout than EPI?

@handwerkerd
Copy link
Member

@all-contributors please add @katielamar for code, design, ideas

@allcontributors allcontributors bot mentioned this pull request Mar 21, 2025
Merged
@allcontributors AllContributors
Copy link
Contributor

@handwerkerd

I've put up a pull request to add @katielamar! 🎉

@handwerkerd handwerkerd merged commit 35c4106 into ME-ICA:main Mar 21, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@handwerkerd handwerkerd handwerkerd approved these changes

@tsalo tsalo tsalo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No BOLD components detected error
3 participants
@katielamar @handwerkerd @tsalo