Fixes convergence/papi logger for distributed vectors #1147
Conversation
ee5f7b4 to
8e173f9
Compare
|
@MarcelKoch, I think this can also be included in the release ? |
|
@pratikvn Yes, but I didn't want to create more pressure with the release than there currently is. |
|
Yes, but I think this is simple enough to review and probably an important one to have to allow users to log convergence data, which is probably one of the most important thing that users care about when using the solvers. |
| std::is_const<T>::value, | ||
| const experimental::distributed::Vector<ValueType>, | ||
| experimental::distributed::Vector<ValueType>>; | ||
| f(dynamic_cast<type*>(linop), std::forward<Args>(args)...); |
There was a problem hiding this comment.
it should check whether dynamic_cast<type*>(linop) is a nullptr
There was a problem hiding this comment.
Could you expand on that? I don't think any of the two branches makes sense in that case, so I could only throw. Is that your suggestion?
There was a problem hiding this comment.
yes, it is. when the linop can not be the type, it will pass the nullptr and lead segmentation fault, I think
There was a problem hiding this comment.
Agree with Mike here, there should be a check which throws if somehow the cast result is a nullptr. Users or other could use this function in completely unrelated contexts (T not distributed::Vector or anything like that).
cf467ef to
f3ff338
Compare
e612ebc to
bf7be95
Compare
|
format! |
| std::is_const<T>::value, | ||
| const experimental::distributed::Vector<ValueType>, | ||
| experimental::distributed::Vector<ValueType>>; | ||
| f(dynamic_cast<type*>(linop), std::forward<Args>(args)...); |
There was a problem hiding this comment.
Agree with Mike here, there should be a check which throws if somehow the cast result is a nullptr. Users or other could use this function in completely unrelated contexts (T not distributed::Vector or anything like that).
- formatting - fixes tests - test for nullptr/wrong type Co-authored-by: Yuhsiang M. Tsai <yhmtsai@gmail.com> Co-authored-by: Terry Cojean <terry.cojean@kit.edu>
61eed09 to
22523f6
Compare
|
format! |
Co-authored-by: Marcel Koch <marcel.koch@kit.edu>
|
Note: This PR changes the Ginkgo ABI: For details check the full ABI diff under Artifacts here |
|
Kudos, SonarCloud Quality Gate passed! |
Codecov ReportBase: 91.47% // Head: 91.57% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #1147 +/- ##
===========================================
+ Coverage 91.47% 91.57% +0.09%
===========================================
Files 535 536 +1
Lines 46287 46308 +21
===========================================
+ Hits 42343 42408 +65
+ Misses 3944 3900 -44
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
This PR fixes the use of the convergence and papi logger with distributed vector. The record logger is not affected by the addition of the distributed types. The stream logger requires significantly more effort to also enable it for distributed types and should probably better be handled in a new logger.
This can wait until #1133 is merged.