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

[oneMKL samples][computed tomography] Revisions, refactoring and addition of a functional verification #2564

Merged

Conversation

raphael-egan
Copy link
Contributor

Description

This PR revises the "computed tomography" oneAPI sample.

The sample itself was revised so that the application creates reconstructed signal/image samples that are real (no longer complex) and so that those generated real values are directly comparable to the input image's (gray-scale-converted) values themselves. These changes are motivated by the desire to introduce a legitimate functional validation step (introduced herein as well), but that need triggered more thorough revision(s) of the sample's source code.

Extensive explanatory comments were added to the source code, explaining the mathematical nature of the operations at play and/or the technical details related to the suggested implementation.

Type of change

  • Implement fixes for ONSAM Jiras

How Has This Been Tested?

This was extensively tested on a PVC GPU (512 EUs) and a SPR CPU using a machine operating Ubuntu 22.04. The default input file as well as others (not added to this PR) were considered along with an extensive range of input parameters. Due to the nature of the operations at play, one simply cannot guarantee that the mean global error will be below the arbitrarily-chosen threshold for any input file and any choice of (supported) input parameters. However,

  • the default operations were verified to pass the added tests by a large margin;
  • the application was augmented with explanatory messages advising how to tweak input parameter(s) to improve accuracy of the results, if found inaccurate for other input file and/or input parameters.

The updated sample's default behavior was also successfully tested on a BMG GPU machine operating Windows 11.

Regarding instructions to test or to run the sample with different inputs, please refer to the application's "Usage" information (printed in case of invalid usage).

@andreyfe1 andreyfe1 self-requested a review January 17, 2025 09:49
@lhuot
Copy link

lhuot commented Feb 3, 2025

@hparks-intel @dbollapr @intelok @nshamszadeh please review this PR

Copy link

@hparks-intel hparks-intel left a comment

Choose a reason for hiding this comment

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

Thank you for cleaning this up! It looks great. I'm just nitpicking a few things.

Copy link

@hparks-intel hparks-intel left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. I don't think it's important to do anything different with the remaining comment about in_pix_len.

@raphael-egan raphael-egan force-pushed the dev/regan/tomography_check branch from eef399f to d76a518 Compare March 20, 2025 01:02
@raphael-egan
Copy link
Contributor Author

@andreyfe1, this is ready for you to review.

@andreyfe1 andreyfe1 requested a review from jimmytwei March 20, 2025 17:49
Copy link
Contributor

@jimmytwei jimmytwei left a comment

Choose a reason for hiding this comment

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

@andreyfe1 Merge PR?

@andreyfe1
Copy link
Contributor

andreyfe1 commented Mar 20, 2025

@jimmytwei, let's wait for 1 more day if there is any late feedback

@jimmytwei
Copy link
Contributor

@andreyfe1 @raphael-egan Please let me know when I can merge this and if it should go into the 2025.1 release. If we can do this asap, that'll be great...

@andreyfe1
Copy link
Contributor

@jimmytwei, the PR is ready to merge

@jimmytwei jimmytwei merged commit 37d7b38 into oneapi-src:development Mar 24, 2025
@jimmytwei
Copy link
Contributor

@raphael-egan @andreyfe1 Merged. Can you also submit the PR to the main branch? I already merged all of the other PRs for the 2025.1 release and if you want your changes to be in the release, you need to submit another PR to main.

@raphael-egan
Copy link
Contributor Author

@raphael-egan @andreyfe1 Merged. Can you also submit the PR to the main branch? I already merged all of the other PRs for the 2025.1 release and if you want your changes to be in the release, you need to submit another PR to main.

See PR#2639.

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.

6 participants