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

Initial implementation of iterative axis alignment #239

Merged
merged 4 commits into from
Mar 2, 2015

Conversation

matze
Copy link
Contributor

@matze matze commented Mar 14, 2014

This PR addresses #237. Waiting for feedback from @Tomago.

@tfarago
Copy link
Contributor

tfarago commented Mar 15, 2014

Looks good but I think we would have much better results if we did flat correction. Have a look how align_rotation_axis is implemented. We could also just pass the flat field frame as a keyword argument. I think I will simplify it there too. The thing is that the user needs to find the flat position anyway so once they are in it they might as well grab a frame and pass it to the respective routines.

I am sure you tested it, that's where the file camera commit came from 😉. Can you post here some plots with the measures you tried? I would like to see how different measures behave. If you don't have them don't bother, we will see it on real data next week.

Anyway, I can help with this too, so if you agree I can implement what I mentioned before and push it.

@matze
Copy link
Contributor Author

matze commented Mar 15, 2014

I am sure you tested it, that's where the file camera commit came from 😉. Can you post here some plots with the measures you tried? I would like to see how different measures behave. If you don't have them don't bother, we will see it on real data next week.

No, I haven't tested yet, because changing the motor position obviously doesn't change a projection. I could, for a test, make the reconstruction axis dependent on the motor position.

Anyway, I can help with this too, so if you agree I can implement what I mentioned before and push it.

Yes, please. Less redundancy = good. I was already pretty surprised how easy this reconstruction chain was to set up but we definitely need some better documentation because all those important things are scattered around.

@tfarago
Copy link
Contributor

tfarago commented Mar 15, 2014

I could, for a test, make the reconstruction axis dependent on the motor position.

That's what I meant, then when you off-center the axis you see the artifacts and can apply the measure and see how it changes.

@tfarago
Copy link
Contributor

tfarago commented Mar 15, 2014

Actually that doesn't make much sense, because then you introduce the ring artifacts (misaligned reconstruction axis), whereas when the sample is away we just see less of it. Let's wait for the real data.
BTW the issue and PR names got me confused, they should be called something like sample-centering I guess.

@matze
Copy link
Contributor Author

matze commented Mar 15, 2014

Actually that doesn't make much sense, because then you introduce the ring artifacts (misaligned reconstruction axis), whereas when the sample is away we just see less of it.

No, no. For 180° tomography we want the axis aligned with the center of the detector, therefore if the reconstruction rotation axis is fixed and we move the actual reconstruction axis we still get the artifacts.

About the name ... well that's again due to the fact that we have two reconstruction axes. The real one and the one assume for reconstruction.

@tfarago
Copy link
Contributor

tfarago commented Mar 15, 2014

Yes, yes 😃 Right, the sample moves with the axis. Anyway, do we actually need to do it iteratively with the devices and tomograph reacquisition? Wouldn't doing it the other way around (changing the reconstruction axis instead of the real one) be simpler?

@async
def find_rotation_axis(sinogram):
    axis_pos = width_2
    while is_better(tomo_slice):
        tomo_slice = backproject(axis_pos)
        axis_pos = calculate_new_axis(tomo_slice)

    return axis_pos

@async
def center_rotation_axis(motor, sinogram, pixel_size):
    motor.position = (sinogram.shape[1] / 2 - find_rotation_axis(sinogram).result()) * pixel_size

(Don't take the code too seriosly, it's just a quick brain dump).

@matze
Copy link
Contributor Author

matze commented Mar 15, 2014

But I'd really much prefer a pixel-perfect alignment to the center. I assume image quality being best in the center, right? I mean stuff like lens distortion and reduced light in the corners.

P.S.: We should actually do both. Perfect alignment for acquisition and sub-pixel precise axis determination for final reconstruction.

@tfarago
Copy link
Contributor

tfarago commented Mar 16, 2014

But I'd really much prefer a pixel-perfect alignment to the center. I assume image quality being best in the center, right? I mean stuff like lens distortion and reduced light in the corners.

I think that +/- a couple of pixels don't matter from the image quality point of view. If you want a high quality reconstruction you can use the simplified method, then acquire another tomograph (with the sample in the center) and calculate the reconstruction axis again.

P.S.: We should actually do both. Perfect alignment for acquisition and sub-pixel precise axis determination for final reconstruction.

I think the sub-pixel precise reconstruction axis is enough. But let's see how things turn out during the beam time. I will also ask another possible users of this stuff.

@matze
Copy link
Contributor Author

matze commented Mar 19, 2014

I like what I see 👍

@tfarago
Copy link
Contributor

tfarago commented Apr 1, 2014

Sorry for the forced push but I had to rebase against master to get rid of the bug dealt with in a16064e.
Now the computation works (at least for my test data).

@matze
Copy link
Contributor Author

matze commented Apr 3, 2014

@Tomago: I'd say it's ready, so if you like you can squash 'n' merge.

@tfarago
Copy link
Contributor

tfarago commented Dec 16, 2014

Simplified and squashed. I will test it today and if that works we can merge.

@matze
Copy link
Contributor Author

matze commented Dec 16, 2014

👏

@tfarago
Copy link
Contributor

tfarago commented Feb 4, 2015

Still needs some work on the backprojection side, shifting.

@tfarago tfarago modified the milestones: Concert 0.11, Concert 0.10 Feb 4, 2015
@tfarago
Copy link
Contributor

tfarago commented Mar 2, 2015

🎆 I changed the filtering to the padded one so the algorithm should be more robust. I tested the compute_rotation_axis. I think it can be merged now.

@tfarago tfarago added ready and removed in progress labels Mar 2, 2015
@matze
Copy link
Contributor Author

matze commented Mar 2, 2015

Great 👍

But on the other hand, we could wait another two weeks and celebrate yet another birthday ;-)

tfarago added a commit that referenced this pull request Mar 2, 2015
Initial implementation of iterative axis alignment
@tfarago tfarago merged commit 6d17ca2 into master Mar 2, 2015
@tfarago tfarago deleted the add-iterative-alignment branch March 2, 2015 17:04
@matze matze removed the ready label Mar 2, 2015
@tfarago
Copy link
Contributor

tfarago commented Mar 2, 2015

This thread will be available also by then. Feel free to do so ;D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants