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

ENH: Reorder the photon counts inside one-step filter #621

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

arobert01
Copy link
Collaborator

When subsets are used, the photon counts are shuffled before doing the reconstruction. The shuffle was done in the application and not in the filter which is not how it is done for the others reconstruction filters.

Copy link
Collaborator

@SimonRit SimonRit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a bug, see here. Do you know what could be the issue? And could you modify the graph to include the new filters in the class doc? Thanks!

Comment on lines 452 to 460
// If subsets are used the photons counts and geometry are shuffled, then geometry need to be set again.
if (m_NumberOfSubsets != 1)
{
m_ReorderPhotonCountsFilter->Update();
this->SetGeometry(m_ReorderPhotonCountsFilter->GetOutputGeometry());
m_ForwardProjectionFilter->SetGeometry(this->m_Geometry);
m_SingleComponentForwardProjectionFilter->SetGeometry(this->m_Geometry);
m_GradientsBackProjectionFilter->SetGeometry(this->m_Geometry.GetPointer());
m_HessiansBackProjectionFilter->SetGeometry(this->m_Geometry.GetPointer());
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I look at the code of ReorderProjectionsImageFilter, I don't see why this is required. It should be sufficient to do it in UpdateOutputInformation no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried by setting the new geometry only in the GenerateInputRequestedRegion() method but then I get this error Mismatch between the number of projections and the geometry entries. Geometry has 0 entries, which is less than the last index of the projections stack. Maybe I'm missing something here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading section 8.3.2 of the software guide, I think the checks on the geometry should be in the VerifyInputInformation and not in the VerifyPreconditions so that the geometry can be modified in the UpdateOutputInformation. Does it tell you which filter complains with this error message?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the error comes from the BackProjectionImageFilter here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to avoid doing this, we need to rewrite ReorderProjectionsImageFilter to create the geometry in GenerateOutputInformation. That's for a different PR I guess, thanks for finding the proposed solution.

When subsets are used, the photon counts are shuffled before doing the
reconstruction. The shuffle was done in the application and not in the
filter which is not how it is done for the others reconstruction filters.
@arobert01
Copy link
Collaborator Author

arobert01 commented Sep 25, 2024

The last force-pushed fix the bug and update the documentation.

@SimonRit SimonRit merged commit 9cbea00 into RTKConsortium:master Sep 26, 2024
59 checks passed
@arobert01 arobert01 deleted the one-step branch October 24, 2024 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants