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

rocSPARSE general functions (part III) #114

Merged
merged 10 commits into from
Jun 10, 2024

Conversation

Beanavil
Copy link
Collaborator

@Beanavil Beanavil commented May 6, 2024

This pull request contains the third (and last) part of the fourth batch of rocSPARSE examples. This group of examples corresponds to the ones that showcase general functions' (not corresponding to a particular matrix format) use. The original PR has been divided into several of them because the total amount of examples to upstream for this batch is too big for only one PR. This third part includes four examples:

  • level 2
    • csritsv
    • spitsv
  • preconditioner
    • gtsv
    • gpsv

@Beanavil Beanavil requested review from a team and dgaliffiAMD as code owners May 6, 2024 14:05
Libraries/rocSPARSE/level_2/csritsv/main.cpp Show resolved Hide resolved
solve_policy,
temp_buffer));

// 6. Perform triangular solve A' y = alpha * x.

Choose a reason for hiding this comment

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

A' ?

Libraries/rocSPARSE/level_2/spitsv/CMakeLists.txt Outdated Show resolved Hide resolved
This triangular solver is used to solve a linear system of the form

$$
A'y = \alpha x,

Choose a reason for hiding this comment

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

Can we use op(A) rather than A' ? This is confusing.

Copy link
Collaborator Author

@Beanavil Beanavil May 7, 2024

Choose a reason for hiding this comment

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

Hmm, this is also present in many other examples from rocSPARSE and in one example from rocBLAS, I propose to update either all of them or none, to keep the notation consistent. If this notation is preferred, I can create an internal issue and we'll upstream the fix when it's done (so in another PR)

Choose a reason for hiding this comment

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

I would suggest to update all of them. Feel free to do it in a separate PR, but A' is not a good notation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Noted 👍🏼. I'll open a separate PR when this is done

@Beanavil Beanavil force-pushed the sparse-general-functions-3 branch from 7ce7864 to 82570be Compare May 27, 2024 12:26
@Beanavil
Copy link
Collaborator Author

@YvanMokwinski I think this is ready to merge, as the changes requested for changing the notation A' to op(A) are going to be applied in a separate PR.

@dgaliffiAMD dgaliffiAMD merged commit 9910763 into ROCm:develop Jun 10, 2024
2 checks passed
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.

4 participants