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.

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

Making heuristic functions a simple set of if elif else statements #44

Closed
smoia opened this issue Nov 28, 2019 · 3 comments · Fixed by #209
Closed

Making heuristic functions a simple set of if elif else statements #44

smoia opened this issue Nov 28, 2019 · 3 comments · Fixed by #209
Labels
BrainHack This issue is suggested for BrainHack participants! Enhancement New feature or request Refactoring Improve nonfunctional attributes

Comments

@smoia
Copy link
Member

smoia commented Nov 28, 2019

Detailed Description

At the moment, heuristic files contain a bunch of code that shouldn't be changed by the user (e.g. from here).
It would be better to move that code outside of the heur file, so that the only content of those scripts is a simple definition that reads an input, goes through a bunch of if statements to set up the labels necessary for the name of the file, and returns those labels.

Context / Motivation

This would simplify the heuristic files on one hand, and help the integration of phys2bids in other BIDSificators programs such as BIDScoin (as in #199).

Possible Implementation

  • Move all the non-if..elif..else code into the function use_heuristics.
@smoia smoia added Enhancement New feature or request Good first issue Good for newcomers labels Nov 28, 2019
@smoia smoia added the Refactoring Improve nonfunctional attributes label Jan 29, 2020
@smoia smoia added this to the phys2bids first non-beta milestone Feb 5, 2020
@rmarkello rmarkello removed the Good first issue Good for newcomers label Mar 12, 2020
@rmarkello
Copy link
Member

I think it's super important that we do this—having just run through the documentation before our call, this was one of my first thoughts so I was happy to see it's an issue already! That said, even I'm not totally sure of how this should be implemented, currently. As such, I removed the "good first issue" label from this issue. Let me know if you strongly disagree (I just wanted to provide a brief rationale for why I removed it).

I do think we can take a look at heudiconv for inspiration as to how they handle their heuristic files! It's been a while since I dug into their codebase so I'm sure it's changed considerably, but last I remember they had some relatively elegant solutions.

@smoia
Copy link
Member Author

smoia commented Mar 12, 2020

I see what you mean!
The reason for which I labelled it as a G1I was because what this issue addresses (in my mind) is just moving some lines of code from within the heuristic files to the function that calls them and gets their output. For instance, moving everything after line 38 of this file, possibly even from line 35, into this function.
The solution could just be changing the heuristic function return, the relative variable initialisation in the use_heuristic function, and move those lines, so that heuristics are just a pure set of if elif else statements.

We will also need to imrpove heuristics in general, but that should come in a different - and definitely not G1I - issue.

@smoia
Copy link
Member Author

smoia commented Mar 12, 2020

Now that I see it, we should reopen issue #45 , that is not a duplicate of this one.

@smoia smoia changed the title Improve heuristic files Making heuristic functions a simple set of if elif else statements Mar 12, 2020
@smoia smoia added the BrainHack This issue is suggested for BrainHack participants! label Apr 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BrainHack This issue is suggested for BrainHack participants! Enhancement New feature or request Refactoring Improve nonfunctional attributes
Projects
None yet
2 participants