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

Check & revise LN2_DEVEIN linear mode #31

Open
ofgulban opened this issue Oct 20, 2020 · 6 comments
Open

Check & revise LN2_DEVEIN linear mode #31

ofgulban opened this issue Oct 20, 2020 · 6 comments
Assignees

Comments

@ofgulban
Copy link
Collaborator

A reminder for myself to revise LN2_DEVEIN 's -linear mode. The output value range seems problematic due to the scaling.

@ofgulban ofgulban added this to the v1.7.0 milestone Oct 20, 2020
@ofgulban ofgulban self-assigned this Oct 20, 2020
@ppxdm4
Copy link

ppxdm4 commented Jul 12, 2021

Hi @ofgulban, I've been looking at this option of LN2_DEVEIN recently and think I'm having the same issue. The 'deveined' profile looks different when you use layer depths 0->1 (metric option) compared to 1->6 (for nr_layers=6). The non-metric case is corrected the profile such that it has a very steep negative gradient.
In the code snippet below (lines 243-259) I believe l = 1 - *(nii_layer_data + i) * layer_max; should actually be l = 1 - *(nii_layer_data + i) / layer_max;

if (mode_linear) {  // Handle linear case straightforwardly
        for (int t=0; t<size_t; ++t) {
            for (int i=0; i<nr_voxels; ++i) {
                int j = t * nr_voxels + i;
                if (*(nii_layer_data + i) > 0) {
                    float l;
                    if (layer_max <= 1.0) {  // Indicates metric file is used
                        l = 1 - *(nii_layer_data + i);
                    } else {
                        l = 1 - *(nii_layer_data + i) * layer_max;
                    }

                    *(nii_output_data + j) = *(nii_input_data + j) * l;
                }
            }
        }
    }

Apologies if this is incorrect or you have already addressed the issue. It seems to have worked with the simple cases I have been testing on and makes sense when I think about how variable l is used.

@ofgulban
Copy link
Collaborator Author

ofgulban commented Jul 20, 2021

Hi @ppxdm4 , Thanks a lot for bringing this to our attention and your informative comment. We ended up not using LN2_DEVEIN over the last year and were postponing looking into this issue. Together with your comment, now we have a reason to address this issue (tagging @layerfMRI as this might be of interest to him too).

@ofgulban ofgulban modified the milestones: v1.7.0, v2.1.0 Jul 20, 2021
@layerfMRI
Copy link
Owner

layerfMRI commented Jul 21, 2021

Thanks @ppxdm4, your fix is now included in the devel branch.
I added another shift of half a layer, to account for the fact that there is no zeroth layer (the average centroid of layer 1 is at 0.5).
And it looks as expected in a quick test.

It is only now that I appreciate what @ofgulban did in 1d5a3d9 (related to issue #28).
It's not a scaling with the (multiplicative) reciprocity of the layers, but a multiplication with the (additive) inverse of the depth. So my earlier comment during the second coffee break is no longer valid. I think this solves my issue about normalizing it to layer 1. I think this issue can be closed at the next merge to master

@ppxdm4
Copy link

ppxdm4 commented Jul 22, 2021

Hi @ofgulban, @layerfMRI, thank you both for these updates. I have pulled the latest devel branch and will be implementing the new linear deveining in the next few days. I have a quick question after reading issue #28:
Could you clarify - when using LN2_DEVEIN with the -linear flag, if I provide a columns file will the corrections be performed on a column by column basis?

@layerfMRI
Copy link
Owner

layerfMRI commented Jul 22, 2021

Hi @ppxdm4,
Yes, I believe that the linear case does not use the column file, if you provide it or not. The linear scaling happens on a voxel by voxel level. This is different to the deconvolution case. Which works on a column by column basis. This was different before @ofgulban revised it in 1d5a3d9.
I am not sure, why this was changed, but I think the results should be comparable for thin columns. The new way is more minimalistic (more safe). The new way does not average within the columns. Only this way it is possible to straightforwardly use the metric file too. The new way of ignoring the columns for the linear case does not have the SNR benefit of local averaging within the layer-column intersection. And thus, it is not easily comparable with the deconvolution case.

@ofgulban
Copy link
Collaborator Author

ofgulban commented Jul 23, 2021

Looking at the previous comments on #28, I think we have changed the columns file behavior due to a few discrepancies between the code and the blog post. Looking at the code, yes the columns file seems to be not used with linear option right now (v2.0.0).

However, this is not a strong limitation. We can code in so that linear option uses columns to first averages the voxels within each column, does linear deveining, and writes back into the columns. If @ppxdm4 want to use linear deveining in such a way, I am happy to look into this in the upcoming days :)

@ofgulban ofgulban removed this from the v2.1.0 milestone Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants