search

UCL / STIR

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

move current flips into GEHDF5Wrapper #636

Open AnderBiguri opened 4 years ago

AnderBiguri commented 4 years ago

I can see it in:

KrisThielemans commented 4 years ago

I'd rather not fix the flips w.r.t. Palak's code in #601. let's finish that one off first. minimal changes still required I hope!

It's not clear to me how this issue differs from #675, but maybe you meant only "move current flips into GEHDF5Wrapper" (which would make sense to me)

AnderBiguri commented 4 years ago

@KrisThielemans Indeed, this issue was opened a while ago (before the current axis shenanigans) and I just meant to put all the flips inside the wrapper, just in case there was a moment where axis flipping shenanigans appear. Then we would have all the flips in a single file instead of scattered around STIR everywhere! (but now we have axis flipping and this issue is still incomplete!)

As you say, this is just to put everything in a single place, making it slightly more modular.

KrisThielemans commented 4 years ago

great.

regarding double-flip. a test is to reconstruct from STIR-unlisted sinograms, and from GE RDF sinograms. results should be the same.

Could do this test with just very few iterations.

AnderBiguri commented 4 years ago

@KrisThielemans will do, but that must be wrong. The flip in ProjDataGEHDF5 was there before I added the one in the Wrapper!

pwadhwa351 commented 4 years ago

The flip in ProjDataGEHDF5 was there before I added the one in the Wrapper!

Just to make sure I understand this properly, there was a (required) tangential flip in ProjDataGEHDF5 class since my code?

I basically read the data to match what we get with listmode, so this might mean the flip must be reversed?

AnderBiguri commented 4 years ago

@pwadhwa351 Yes, that is right. When I started working on this code, the following was in ProjDataGEHDF5 (it was called ProjDataFromGEHDF5, but the following file is the same, with the name changed): https://github.com/UCL/STIR/blob/eb552aaa07fc3cabf35d68a379af1dde94c68ab6/src/buildblock/ProjDataGEHDF5.cxx#L165

I have tried to change only code, not function, so I always tried to keep the flip. At some point, I decided to include all flips inside the wrapper, just to put everything in the same place. But I apparently forgot to remove it from ProjDataGEHDF5! Now its in both the wrapper and there, so its double flipping.

But in any case, considering the axis flipping confusion we are having, I will do the same, try to match it with the listmode, make sure that I am reading it the same way.

AnderBiguri commented 4 years ago

regarding double-flip. a test is to reconstruct from STIR-unlisted sinograms, and from GE RDF sinograms. results should be the same.

Could do this test with just very few iterations.

Hum, the VQC phantom data seems to suggest that there is no double flip.

KrisThielemans commented 4 years ago

There's https://github.com/KrisThielemans/STIR-1/blob/2500cca678fd6a8a49c2de813fd20f6622c3be38/src/IO/GEHDF5Wrapper.cxx#L714-L734 and https://github.com/KrisThielemans/STIR-1/blob/2500cca678fd6a8a49c2de813fd20f6622c3be38/src/buildblock/ProjDataGEHDF5.cxx#L155-L169

They have different purposes: the first one handles handles the bug in RDF9, the second one coordinate axis stuff. So I don't see a double-flip

AnderBiguri commented 4 years ago

@KrisThielemans you are correct. The first one is not a flip, its a permute, so there is no "double flip". I just got confused.

The second one flips the view and the tangential axis, but does not "permute" axial and tangential indeed, which is what the first does.

Now, in theory I can add the tangential "flip" into the Wrapper, and remove it from the second line you linked.

KrisThielemans commented 4 years ago

This isn't just about tangential flips. As I wrote in https://github.com/UCL/STIR/issues/675#issuecomment-688387335, there are a whole set of "flips" going on.

It seems that we're currently not clear which functions/data are in which convention, with the occasional mixture (as an example, inferring from the BinNormalisationFromGEHDF5::initialise_geo_factors_data code, initialise_geo_factors_data takes a GE-view, sets up offsets using GE "combined axial position", calls read_geometric_factors which does a tang-pos-flip, and copies into a STIR viewgram with STIR coordinates.

It's a bit daunting to go through all this...

I guess we could either:

Not so sure what the best (and feasible) option is.

AnderBiguri commented 4 years ago

@KrisThielemans The one I like most is keeping everything in the wrapper. Not sure how easily we can do this, but conceptually, it makes sense that the data reader not only knows how to read the bytes, but also outputs the data in the convention that the software uses. This, I think, will also make long term support much easier.

KrisThielemans commented 4 years ago

Arguably, the name of the class is closer to second option (just a wrapper around HDF5 functionality), but we can rename it of course. Anyway, as I believe this choice is largely a matter of taste, the question is which of those 2 options is the least amount of work ...