Mixed Precision ISAI #719
Closed
Mixed Precision ISAI #719
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
format! |
Codecov Report
@@ Coverage Diff @@
## develop #719 +/- ##
========================================
Coverage 94.17% 94.18%
========================================
Files 400 400
Lines 31051 31084 +33
========================================
+ Hits 29241 29275 +34
+ Misses 1810 1809 -1
Continue to review full report at Codecov.
|
thoasm
previously requested changes
Mar 23, 2021
There was a problem hiding this comment.
There are some parts that I would really like to change before merging this PR.
Additionally, I realized that it might make sense to make SpdIsai and GeneralIsai separate classes or at least specialization of the current Isai because:
- they seem to not share too much with
LowerIsaiandUpperIsai - there is a lot of branching in all functions
- all
Isaipreconditioner (includingLowerIsaiandUpperIsai) are stored in ELL in the current implementation get_approximate_inversehas a lot of overhead now
Comment on lines
255
to
252
| template <typename OutType, typename InType> | ||
| std::shared_ptr<OutType> convert_matrix_formats( | ||
| std::shared_ptr<InType> in) const | ||
| { | ||
| if (std::is_same<OutType, InType>::value) { | ||
| return as<OutType>(in); | ||
| } | ||
|
|
||
| auto out = OutType::create(in->get_executor()); | ||
| if (std::is_same< | ||
| InType, gko::matrix::Csr<typename InType::value_type, | ||
| typename InType::index_type>>::value) { | ||
| auto tmp = gko::matrix::Csr< | ||
| typename OutType::value_type, | ||
| typename InType::index_type>::create(in->get_executor()); | ||
| as<Csr>(in)->convert_to(tmp.get()); | ||
| tmp->convert_to(out.get()); | ||
| return share(out); | ||
| } else { | ||
| auto tmp = gko::matrix::Ell< | ||
| typename OutType::value_type, | ||
| typename InType::index_type>::create(in->get_executor()); | ||
| as<Ell>(in)->convert_to(tmp.get()); | ||
| tmp->convert_to(out.get()); | ||
| return share(out); | ||
| } | ||
| } |
There was a problem hiding this comment.
I have some problems with this function:
- It should be private to not be part of the public interface
- It needs documentation
- The function can only convert CSR <-> ELL and not arbitrary conversions (which the use of templates suggest)
|
SonarCloud Quality Gate failed. |
tcojean
reviewed
May 28, 2021
|
Kudos, SonarCloud Quality Gate passed! |
upsj
reviewed
Jun 10, 2021
There was a problem hiding this comment.
LGTM in general. To be honest though, I would rather want to wait for full mixed-precision support in Csr instead of choosing Ell just because it is the only type where we put in the effort to make it mixed-precision capable. Coo would probably also be alternative with low development effort for the changes, and could provide much better worst-case performance.
Comment on lines
+123
to
+125
| Isai<IsaiType == isai_type::lower | ||
| ? isai_type::upper | ||
| : IsaiType == isai_type::upper ? isai_type::lower : IsaiType, |
There was a problem hiding this comment.
could we move this into a
constexpr isai_type transpose(isai_type type) {
return type == isai_type::lower ? isai_type::upper : type == isai_type::upper ? isai_type::lower : type;
}
function?
Suggested change
| Isai<IsaiType == isai_type::lower | |
| ? isai_type::upper | |
| : IsaiType == isai_type::upper ? isai_type::lower : IsaiType, | |
| Isai<transpose(IsaiType), |
|
I think this PR may be obsolete with Csr now being mixed-precision capable? |
|
Yes, I will close it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes the ISAI preconditioner support reduced precision storage format.
It is based on Tobias' mp spmv approach for ell, which is slightly changed to use an accessor on both the matrix values and the input vector, for now always using the ValueType of the output vector as arithmetic type.
Points I would like to discuss are:
GKO_INSTANTIATE_FOR_EACH_VALUE_INDEX_AND_STORAGE_TYPEin types.hpp to make this work. I'm not really happy with having this additional to the mixed value and index types Tobias already introduced, maybe we can find a better way.Todos: