adding tests. simplifying getfbounds

[handwerkerd]

@katielamar I'm still working on this, so don't merge yet, but I figured I'd share.

Once nice thing about writing tests is that I notice other issues. In trying to create the test, I realized that getfbounds and a few functions that just called that were being passed n_echos and echo_dof, & using only one for the exact same thing. Writing a test for how to handle n_echos vs echo_dof didn't make any sense. Since this is a statistical function and the parameter for the statistic is the DOF, i decided to replace n_echoes with echo_dof in all those functions. In the functions that call it, echo_dof=n_echoes when echo_dof==None.

I still have at least one more test to add so I'm setting this as a draft PR.

[handwerkerd]

@katielamar I think everything is being tested now and all tests are passing. Happy to discuss here or by voice.

[katielamar]

Thanks! I’ll review the changes made tonight and let you know if I have any questions. We can probably just discuss on here if that’s easier.

[katielamar]

@handwerkerd this looks great. I think I've got a better idea on how to write the tests now. I think I was focused more on making sure the output fstats matched a ground truth rather than doing some sanity checks such as making sure fstat values using an echo_dof < # of good echoes is smaller than when using # of good echoes as the degree of freedom.

My only suggestion is regarding the comment lines on the fstat test. I think they could be a bit clearer if we changed it to:

line 231:
When echo_dof < the number of echoes, then the f_maps_orig should have the same or larger values
line 234:
# When echo_dof==3 and there are 5 good echoes, then the f_maps_orig -f_maps should always be larger than 0

Let me know what you think and I can merge/make the changes.

[handwerkerd]

I just fixed the comments (keeping them <100 characters to comply with the style guide). The one thing I can't do is run this on actual EPTI data so, once you know this works on your data, you can merge & then I'll review & approve.

FWIW, I prefer tests that specify precise outputs and I try to include such tests in everything new I add to tedana, but the project was started using more smoke tests that just make sure things ran as expected without crashing and other tests that verify errors happen when expected.

If you look at https://github.com/ME-ICA/tedana/blob/main/tedana/tests/test_external_metrics.py, which I wrote from scratch when we added the external metrics options, I'm explicitly testing the outputs of key functions. I'd love to see more of this going forward, but it's a lot of thankless work to retroactively add specifications to pre-existing tests. I try to improve older tests when I have reasons to interact with them.

The test for calculate_f_maps is a case where we were inputting random data to make sure it ran. I decided to specify that we know how echo_dof would alter the relative values, but I decided not to expand the scope by creating a new fixed input so that we could test the precise outputs of both. (This is possible & I'm rethinking my decision, but I'd still prefer to just get this merged). There were also other functions you changed which were only being evaluated by integration tests that run the full tedana pipeline. I decided to add echo_dof to one of our integration tests rather than create expansive tests for additional functions in this nicely focused pull request.

[katielamar]

Sounds good. I'll go ahead and run a few tests with my data to verify everything looks okay. It will take about a day to run everything. I'll merge after that.

[katielamar]

@handwerkerd , I just realized that some tests aren't passing anymore due to the assert in the fstat maps test requiring that

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

instead of:

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

I updated the code and Im rerunning the tests locally. Once they are passing Ill push.