Make heuristics a simple set of "if .. elif .. else" statements and use a dictionary instead of variables.

[smoia]

Closes #89, #44

Proposed Changes

• All the heuristics set run to run-01 rather than run-00 (closes 0- vs 1-indexing the run keyword in use_heuristic() #89 )

• The tests have been updated consequently to this change

• All the "functional code" of the heuristics moved to phys2bids.use_heuristics rather than in the heuristic file, leaving it cleaner for the user (closes Making heuristic functions a simple set of if elif else statements #44 ).

• Instead of variables, the heuristic file populates a dictionary (this will be used to read yaml files too).

• The documentation has been updated to reflect the changes.

Now, I have two doubts:

1. Is this a breaking change? It discontinues support for older heuristics, although not that much.
2. I added a docstring to the heuristic file for completeness, but in this way the first code that the user should change comes after a long while - and a whole lot of information that probably is not necessary to them. I wouldn't see anything bad in removing the docstring in the heuristics to decrease the possibility of confounding the user, as in any case there's a full tutorial on how to use this function, we're not testing it, and we're not going to report it in the API.

[codecov]

Codecov Report

Merging #209 into master will decrease coverage by 0.13%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##           master     #209      +/-   ##
==========================================
- Coverage   94.46%   94.32%   -0.14%     
==========================================
  Files           7        7              
  Lines         578      582       +4     
==========================================
+ Hits          546      549       +3     
- Misses         32       33       +1     

Impacted Files	Coverage Δ
phys2bids/phys2bids.py	90.00% <90.90%> (-0.45%)	⬇️

[eurunuela]

LGTM! 👍

I really like the idea of using a dictionary to have the BIDS keys accessible at all times. It makes the code much easier to read imo.

I'm not worried about the decrease in coverage. I think we can ignore it this time.

[eurunuela]

I assume this one is an example?

[smoia]

Yep, why not.
it's the one used for a dataset that will be shared at a certain point.

[smoia]

@eurunuela I addressed your point - hopefully also the coverage decrease.

@rmarkello @RayStick I had the same issue as #208 related to the deleted file. I think I solved it but @RayStick and @vinferrer please confirm that for our aims this old file: https://osf.io/u5dq8/ and this new file: https://osf.io/5829m/ are the same.

[rmarkello]

Hey @smoia! Thanks for all this 🙌

I made a few (very minor) suggestions below, but otherwise this looks good 💯

One lingering question that this all raised for me, though: is there a way to handle the heuristic files such that users don't have to specify the exact run number if they have multiple runs of the same task? Could we use a list to store the info and just loop through? I know heudiconv uses an approach like that so I'm wondering if it would be feasible here!

That said, I think we can merge this as-is in and open another issue to address this stuff more broadly!

[RayStick]

@rmarkello @RayStick I had the same issue as #208 related to the deleted file. I think I solved it but @RayStick and @vinferrer please confirm that for our aims this old file: https://osf.io/u5dq8/ and this new file: https://osf.io/5829m/ are the same.

Apologies for that @smoia. I deleted them for a reason, but it was a stupid reason. Instead of explaining, I have actually just put those text files back now. They may be slightly different lengths (i.e. different number of samples) compared to the original file, but otherwise they are the same. You can use either of those files, that you linked to (though 'Test3' will have have a new hyperlink now). Let me know if there are any issues.

[smoia]

@RayStick, no worries! If it's shorter than the one we were using before and it has the same characteristics as the previous one that we use to test, then it's even better!
I just want to be sure.

@rmarkello, it's a great idea and we should implement it - not only for heuristics but also for mappings!
I would close this PR as is (with the suggestions you gave), and open an issue to add that feature (that will come extremely helpful once #206 is complete).
The only doubt that I still have is about the docstring in the heuristic file. Maybe it's not only not necessary, but also problematic. But if you think it's good as it is, think about which label to apply to this PR (if it's a breaking change or not - if it is the label is "majormod"), and merge it in!

[rmarkello]

@smoia: a few thoughts on the heuristics files, then!

1. I vote to remove the logger import and statement from all the examples. Handle this inside phys2bids (i.e., if task isn't set then raise an error).
2. It is unclear from the doc-string in the heuristic files that the parameter physinfo is the filename. You currently say "Name of the file or partial match" but it's always a filename. It would be great to clarify this (as you have done in the documentation)!
3. Is it possible to avoid pass info to the heuristic function? I know it comes from the calling function but can't you just define info as a dict inside the heur() function, update it accordingly, and then update the info dict in the calling function? I'm thinking something like:

filename = 'my_physio_data.txt'
info = {'sub': '001', 'ses': '01'}
info.update(heuristic.heur(filename))

That way heur() only has one parameter (the filename, for now—eventually more info!).

Let me know what you think.

[smoia]

@rmarkello good ideas! I implemented them in the last commit.
I'm not totally sure about initialising the dictionary in the heuristic file vs passing it as an argument, but for sure updating the dictionary is a much better idea!

[smoia]

Ok, codecov is not happy about the decrease in testing due to the new line that is in the function.
Since we don't have breaking tests (do we?), I would move on and merge it anyway, if @rmarkello you're happy with the latest version.

[rmarkello]

This is great! Thanks so much for this @smoia !

[rmarkello]

This might now count as a breaking change... Let me know and I'll update the labels + merge in accordingly.

[smoia]

@rmarkello I trust you - up to you to decide which label to add (majormod or minormod) and merge in!