-
Notifications
You must be signed in to change notification settings - Fork 94
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
Add chebyshev Iteration #1289
base: develop
Are you sure you want to change the base?
Add chebyshev Iteration #1289
Conversation
Do you intend adding an eigenvalue estimation? I think that would be very helpful, because most times users don't have that available. I think PETSc is also doing that. I think this is the GMRES version used by PETSC to compute the estimate: https://petsc.org/release/docs/manualpages/KSP/KSPAGMRES/ |
@MarcelKoch thanks for providing these reference. I do not think I will put them in this pull request. From https://doi.org/10.1137/0907057, they introduce the algorithm From https://petsc.org/release/docs/manualpages/KSP/KSPCHEBYSHEV/#kspchebyshev, For GMRES, we need to get the Hessenberg out and compute eigenvalue on it. |
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides the comments left below, I want to mention the num_keep
and num_generated
mechanic. That needs some explaination, because right now I don't understand what that is for. It seems like some sort of restart mechanic, but I might be wrong there. Also, exposing this to the user seems confusing to me.
/** | ||
* The number of scalar to keep | ||
*/ | ||
int GKO_FACTORY_PARAMETER_SCALAR(num_keep, 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not clear what this parameter is for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this means, construct Chebyshev polynomials until degree num_keep
, and after that just do IR with these polynomial?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, num_keep
is to keep the generated scalar in the storage.
alpha and beta are changed by iteration, but they are fixed by the given bound.
It's used for fixed iteration runs because we do not need to refill these scalars for different apply
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But shouldn't we then just keep all scalars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when using it as normal solver, we may not have the maximum iteration information.
allocating one big dense matrix and then moving them to another one when full may not be a good approach.
we can also add the ability of workspace such that it can handle the std::vector then it should be more flexible for uncertain size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can not assume that there's an iteration criterion. It can contain the residual norm or time criterion.
If we assume that, we should provide the standalone iteration parameter.
Users do not need to know the implementation details.
The statement is to keep how many scalars for chebyshev to avoid the refill overhead.
I can introduce this usage later when we have the use case.
I think it should help the situation when kernel launch overhead is noticeable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, this is very useful to negate the overhead. Still, I think we can assume that there is an iteration criterion somewhere in the combined one and use that. If not, we would just throw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could set the default value to unspecified
, which you define before. When the solver is generated, you can check if the parameter is unspecified
and then try to extract an iteration criterion from the criteria. If that doesn't work, you throw with a message to either pass an iteration criterion or set this parameter to something else.
Or alternatively, just replace the criteria parameter with a iterations
parameter and move the class into the preconditioner namespace. I think that is also a fine option, since that would be the main use case anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have moved it to based on the given iteration from stopping criterion.
It is only increased after creating object.
I also think whether staying a fixed number is enough or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think it should be fine the way it is now.
/** | ||
* Chebyshev iteration is an iterative method that uses another coarse | ||
* method to approximate the error of the current solution via the current | ||
* residual. It has another term for the difference of solution. Moreover, this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* residual. It has another term for the difference of solution. Moreover, this | |
* residual. The solution is then updated using the Chebyshev polynomials. Moreover, this |
Is that what the sentence is trying to say? Anyway, I think it should be mentioned somewhere that this uses these polynomials.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the solution x_i is also based on the
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I don't see the relevance of that. Has that any effect for the user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, I only try to explain the algorithm's difference from IR. IR uses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this has to be rephrased
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I try to rephrase it again. Could you take a look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with merging this PR when corrections in parts of the documentation that are pointed out (comments) take place. I could understand what num_keep variable was used for but perhaps a more descriptive name would be better.
c88ea63
to
f9d0e28
Compare
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I would like to resolve some of my earlier issues. Other than that only minor nits.
/** | ||
* Chebyshev iteration is an iterative method that uses another coarse | ||
* method to approximate the error of the current solution via the current | ||
* residual. It has another term for the difference of solution. Moreover, this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this has to be rephrased
Kudos, SonarCloud Quality Gate passed!
|
3f588f5
to
77a3551
Compare
Error: The following files need to be formatted:
You can find a formatting patch under Artifacts here or run |
Co-authored-by: Marcel Koch <marcel.koch@kit.edu>
Co-authored-by: Marcel Koch <marcel.koch@kit.edu>
Co-authored-by: Marcel Koch <marcel.koch@kit.edu>
Co-authored-by: Marcel Koch <marcel.koch@kit.edu>
Co-authored-by: Tobias Ribizel <ribizel@kit.edu>
@upsj I have changed it to accept the host scalar now, so we do not need to store them in advanced. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mainly want to discuss the need for mixed precision instantiation here
namespace chebyshev { | ||
|
||
|
||
template <typename ValueType, typename ScalarType> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this actually need a second precision, or can we always use the highest precision available? This is memory-bound after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the same thought and thinking maybe we just use double or complex directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a numerical benefit to using a higher-precision multiplication every time, I would go for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after rethink about it, I do not have a good way to use double or complex double without any information from the vector or matrix. Maybe still keep the same way as how we handle IR/richardson now. also I need the ScalarType, which is the ValueType from Chebyshev class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently keep the value type of class but change fosi, alpha, beta type to double/complex
when dpcpp only support float not double, it will cast to float/complex internally to keep the same interface among all backends
Co-authored-by: Tobias Ribizel <mail@ribizel.de>
It adds the Chebshev iteration in https://en.wikipedia.org/wiki/Chebyshev_iteration
The second-order richardson uses a similar formula but the scalars are constant in all iteration. Chebyshev Iteration update the scalar from the previous one (the scalar are the same if upper/lower eigval does not change)