search

UCL / STIR

Software for Tomographic Image Reconstruction
http://stir.sourceforge.net/
Other
116 stars 95 forks source link

Arc Correction fail with Signa #774

Open NikEfth opened 3 years ago

NikEfth commented 3 years ago

https://github.com/UCL/STIR/blob/f5025b2895861327f2d54604e026c0deddc857e2/src/buildblock/Scanner.cxx#L355

Due to this bin size the test_ArcCorrection.cxx fails on tang positions -91 and 91. Either with TOF or without.

The older (I think) value of 2.1306F allows the test to pass.

NikEfth commented 3 years ago

Alternatively, if you want to keep the value of 2.01565F you can change the epsilon in the overlap_interpolate to be 10K smaller than the minimum average box value.

KrisThielemans commented 3 years ago

Interesting. I don't understand why you have this and we've never seen it before, but it could just be border line numerics I guess.

I can imagine that if the bin-size is too small or too large, the test starts to fail, although I would have hoped not. @NikEfth can you put to the line with the epsilon?

The number entered for the Signa is probably almost arbitrary. I don't know what GE uses, @pwadhwa351 do you? Possibly they don't use it at all (FBP is getting a bit outdated).

The annoying thing of course is that in STIR you have to specify voxel-size via zoom (and it then uses the arc-corrected bin-size to find the voxel size), so we'll be change the voxel-sizes.

652 suggests to make a default, which would have prevented this.

NikEfth commented 3 years ago

@NikEfth can you put to the line with the epsilon?

What do you mean?

KrisThielemans commented 3 years ago

oops. sorry. Can you give me a link to the line with the epsilon that you referred to above?

NikEfth commented 3 years ago

Here it is:

https://github.com/NikEfth/STIR/blob/76994bef4482a9e83dfd3d7cbad9877ac35fd43d/src/include/stir/numerics/overlap_interpolate.inl#L87

KrisThielemans commented 3 years ago

right. that's less obvious than I had hoped 😁 .

Did you in fact find a numerical instability in overlap_interpolate?

KrisThielemans commented 3 years ago

or in the test I guess

pwadhwa351 commented 3 years ago

The number entered for the Signa is probably almost arbitrary. I don't know what GE uses, @pwadhwa351 do you? Possibly they don't use it at all (FBP is getting a bit outdated).

I did spend some time to find the bin size that GE uses but unfortunately we don't really know.

NikEfth commented 3 years ago

Yes, It was only in the +/- 91 tangential positions. I guess it is to be expected, that when you use thresholds there is a possibility that some overlaps will not work, as rare that that might be is can happen. The failure is at the uniformity test and the return value was 0.9997 instead of 1.

NikEfth commented 3 years ago

Which makes me think should we undo the hardcoded scanners in the tests, make them parametric with default scanner values those that currently are hardcoded, but encourage the user to pass all the tests with his/hers intended scanner template?

KrisThielemans commented 3 years ago

The older (I think) value of 2.1306F .

that's indeed the value entered by @Ottavia, but she probably just used another arbitrary value (it's close to the one from the RX for instance).

I wouldn't change the bin-size to an arbitrary value that happens to make the test not fall over. (it's still a mystery to me why this suddenly starts failing in TOF, while not on any of the other branches, including master). (The reason that test_ArcCorrection failed in #304 is that it uses the Signa, while on master it doesn't).

Because of the zoom stuff, I'd prefer not to change this bin-size.

I guess it is to be expected, that when you use thresholds there is a possibility that some overlaps will not work, as rare that that might be is can happen. The failure is at the uniformity test and the return value was 0.9997 instead of 1.

I cannot really remember why overlap_interpolate needs that epsilon (and I haven't checked). I guess it makes sense that if it's set to size/1000, the numerical error can be of that order of magnitude. I think we can indeed safely reduce the epsilon, and/or reduce the required accuracy in the test (.03% is roughly what we expect overall anyway as we're working with floats).

KrisThielemans commented 3 years ago

Which makes me think should we undo the hardcoded scanners in the tests, make them parametric with default scanner values those that currently are hardcoded, but encourage the user to pass all the tests with his/hers intended scanner template?

Some of the tests can take an input file, but use a default one if you don't specify a scanner. test_ArcCorrection. Uses hard-wired scanners, because really it shouldn't depend on the scanner, but of course, now it turns out that it did (although incorrectly).

I'd be possible to let test_ArcCorrection optionally read a file, and run the test on those dimensions of course. However, life is short...