search

cabouman / mbircone

BSD 3-Clause "New" or "Revised" License
11 stars 9 forks source link

Clear ambiguity about num_slices in documentation #11

Closed smajee closed 3 years ago

smajee commented 3 years ago

In svmbir, number of slices in the sinogram and reconstruction are always the same and a single parameter name is suffcient.

In cone-beam, the number of slices in the reconstruction can differ from that of the reconstruction. Therefore we need different parameter name to differentiate the two. Perhaps num_slices, num_rows_detector?

cabouman commented 3 years ago

I'm not sure I agree. Here is what I suggest.

The number of detector rows should not be an input. The code can get this value from the sino array size. That is how svmbir works. We can informally refer to this is the num_rows in the documentation. Perhaps we should change the svmbir documents to refer to this is num_rows also.

Then the number of reconstructed slices is they only input parameter. This can be called num_slices.

smajee commented 3 years ago

The recon function in mbircone does NOT take the number of detector rows as an input and it is inferred from the shape of the sinogram data (just like svmbir)

However, the documentation states that the sinogram should have a shape of (num_views, num_slices, num_channels) and the reconstruction would be a shape of (num_slices, num_rows, num_cols) just like svmbir. Notice that num_slices appears in both cases with a different meaning.

For svmbir, this is not a problem since the number of slices in the reconstruction always equal the number of slices in the sinogram. For mbircone, this is not the case and can cause confusion to a person reading the documentation (even though there is not a num_slices input).

smajee commented 3 years ago

Based on the discussion with Prof. Bouman today, the conclusion was that the sinogram should be defined as an array of (num_views, num_rows, num_channels) so that num_slices is not used in two places with different meaning.

However, I realized later that this is also problematic since num_rows refers to the number of rows in each slice of the reconstruction. I think we need to come up with a better name.

cabouman commented 3 years ago

How about using num_det_rows as you variable?

smajee commented 3 years ago

num_det_rows sounds good to me.