-
Notifications
You must be signed in to change notification settings - Fork 0
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
Package initial version #3
Conversation
@Laura2305 I've managed to get everything to run through on the example data, with some requirements noted down that I had to figure out, but all good. I will add my review by Thursday. |
@Laura2305 I would like to propose some code changes in a new branch as part of the review, which you can then merge into setup and we can proceed with the PR. To do this I need write access to the git repo (and also I have some similar changes for the example). Could you give me write access? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone through and finished my review of the code. In general it looks great, but I have proposed some changes in my branch #4. Please take a look at my proposed changes, they are mostly related to a function name and adding the REPL dependency explicitly, as well as the filtfilt call with less than 5 slices. These three topics must be addressed, but the formatting changes are up to you if you'd like to keep them or not.
|
||
SCT reference: https://spinalcordtoolbox.com | ||
""" | ||
function callSCT(params::Dict{Symbol, Any}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a warning or a check for sct installation present, as well as fsleyes
src/Unwrap.jl
Outdated
padd_size = size(correlation, 1) | ||
corr_padd = zeros(Float64, padd_size) | ||
corr_filt = cat(corr_padd .= correlation[1], correlation, corr_padd .= correlation[end], dims=1) | ||
corr_filt = filtfilt(filter, corr_filt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The call to filtfilt
can fail for small numbers of slices, see my PR for a possible workaround or we just check if the sizes are compatible and throw an error if they are not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this makes sense. We can just skip that part and use the correlation sign for a small number of slices.
@@ -0,0 +1,54 @@ | |||
|
|||
function test_AdjustData(datadir::String) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good atomic test
end | ||
|
||
|
||
function testdata(datadir::String, tmpResdir::String) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good testset in general. Maybe we should look at setting up a CI run that checks code coverage
Update CI to use a julia version within the scope of MRINavigator's compat
@Laura2305 The failing CI runs are fixed in my branch. I've changed the CI to use Julia 1.3 and 1.8, let me know if this is alright |
1.3 isn't supported by MRIFiles it seems... changing to 1.6
@alexjaffray that's great. Thanks! |
Proposed formatting changes and Function redefinition
fix filtfilt for low slices number
No description provided.