Skip to content

Modify and update the code related to the PICCS algorithm module. #650

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

coldly01
Copy link

@coldly01 coldly01 commented Apr 1, 2025

Fixed bugs in the existing PICCS algorithm CUDA files and added additional code required to invoke the algorithm.

Copy link
Member

@AnderBiguri AnderBiguri left a comment

Choose a reason for hiding this comment

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

Thanks a lot! super useful.

Can you please answer the questions, so I understand better?

@@ -55,7 +55,7 @@ Codes : https://github.com/CERN/TIGRE
#include "TIGRE_common.hpp"
#include "GpuIds.hpp"

void piccs_tv(const float* img,const float* prior, float* dst,float alpha, float ratio, const long* image_size, int maxIter, const GpuIds& gpuids);
void piccs_tv(float* img,float* prior, float* dst,float alpha, float ratio, const long* image_size, int maxIter, const GpuIds& gpuids);
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove const here?

Copy link
Author

Choose a reason for hiding this comment

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

This modification is an attempt I made while debugging the incorrect return value of this operator. Keeping const is a better choice.

Copy link
Member

Choose a reason for hiding this comment

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

@coldly01 makes sense :) can you put it back then?

@@ -128,6 +130,10 @@ def __init__(self, proj, geo, angles, niter, **kwargs):
def run_main_iter(self):
stop_criteria = False
n_iter = 0
if (self.regularization=="PICCS"):
# Modify the `set_res` function in the `Python\tigre\algorithms\iterative_recon_alg.py` file as needed.
res_prior = copy.deepcopy(self.res)
Copy link
Member

Choose a reason for hiding this comment

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

hum not sure about this. res is the current variable right? You may want to initialize the algorithm to one image, but have a prior of another, so I think there should be a new kwarg for prior

Copy link
Author

Choose a reason for hiding this comment

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

My approach is to add the following code in the set_res function.

if init == "PICCS_priorimage":
                file_path='./fdk/fdk_360ang_test.raw'
                data = np.fromfile(file_path, dtype=np.float32)
                nVoxel = self.geo.nVoxel
                self.res=data.reshape(nVoxel) 

And then call os_asd_pocs in demo.py like this.

os_asd_pocs_output=algs.os_asd_pocs(proj, geo, angles, niter, regularization="PICCS", alpha=piccs_alpha, ratio=piccs_ratio, init="PICCS_priorimage", gpuids=gpuids)

Of course a better approach would be to add a kwarg with a name like prior_image_filepath

Copy link
Member

Choose a reason for hiding this comment

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

I think that should be the right way.

In MATLAB we have PICCS as a separate function, ideally we want to have an alias (like os_asd_pocs) for piccs and os-piccs that force you to have a kwarg of prior_image, but not a filepath, but a volume of the same shape as geo.nVoxel.

Let me know if you want to implement this change, or want me to do it before the merge (I will make commits in your branch if so).

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