-
Notifications
You must be signed in to change notification settings - Fork 1
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
Refactor custom heuristics #216
Open
kaitj
wants to merge
16
commits into
khanlab:main
Choose a base branch
from
kaitj:maint/refactor-heuristics
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This changes the heuristics dropdown under the study config to a text box similar to .bidignore, allowing for users with access to update the heuristic as needed.
Heuristic column should not be nullable. Refactored slightly to set default to be whatever is stored under study.custom_heuristic, and making the column non-nullable.
kaitj
force-pushed
the
maint/refactor-heuristics
branch
from
February 29, 2024 18:53
dbd4dc9
to
07f8280
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm happy to leave things as is, but wanted to implement this in a branch if we wanted to change how custom heursitics are provided. Resolves #213.
For a study to use a custom heuristic, a user / admin needs to upload it to the heuristics repository via a PR and it needs to be selected from the study config dropdown. In addition to requiring a maintainer to approve and merge the PR (which I think should be kept in place since every study heuristic is exposed here), an admin also has to manually hit the "update custom heuristics" button on the portal for the heuristics to be immmediately updated. Alternatively, the user can wait until the next day, at which point the cron job to update the heuristics should have run. This provides a bit of a safety net incase there are issues, but there is still a burden on the admin as well as a delay in finding out whether or not an update to a custom heuristic would work.
This PR refactors the custom heuristic to be stored in the database, similar to the custom
.bidsignore
. If a custom heuristic isn't defined, it will usecfmm_base.py
as it was before. If a custom heuristic is required, instead of the drop down, users can input their custom heuristic directly to the study config. At the time whentar2bids
is run, the custom heuristic file will be written out to a temporary file to be read. In doing this, any cron jobs or buttons related to updating an externally saved heuristic can be eliminated and also removes some of the admin burden. Some downsides to this:I've included a screenshot of what this looks like (testing locally).
The template can be found here, which strips all of the custom content, but provides a starting point.