Improve CSR strategies #379

thoasm · 2019-11-12T20:24:55Z

This PR contains the following fixes:

All conversions to CSR (except CSR to CSR) now conserve the original strategy (test for that are included)
CSR strategies now try to copy the least amount of data
Add exception throwing in case the load_balance strategy would not work properly (when srow was empty in the CUDA kernel)
An empty Dense matrix can now be converted to a CSR matrix

Slightly unrelated fix:

the macro GKO_NOT_SUPPORTED does not throw by itself (the only macro that behaves like this, the others include the throw). Now, the macro does include the throw, so it performs as expected.

include/ginkgo/core/matrix/csr.hpp

tcojean

LGTM. I believe only the hybrid read is missing.

core/matrix/hybrid.cpp

include/ginkgo/core/matrix/csr.hpp

Code changed since last review quite a bit.

thoasm · 2019-11-13T03:42:21Z

@ginkgo-project/developers
As part of this PR, I saw that GKO_NOT_SUPPORTED is the only macro that does not throw the error. Instead, it just builds it. I fixed the current code that was missing that throw by adding it, because it would theoretically break the interface if we change the macro (it is in a public header).

Or do we consider this macro to be faulty, in which case we might be allowed to change the macro?

upsj

LGTM, only small nits to pick :)

Question, I'm not yet familiar with the CSR kernels: Would we need to test the conversion to load_balance as well? Or does it use the same data structures as merge_path?

core/matrix/dense.cpp

include/ginkgo/core/matrix/csr.hpp

reference/test/matrix/csr_kernels.cpp

tcojean · 2019-11-13T10:09:23Z

@thaosm about the GKO_NOT_SUPPORTED macro, I'm not sure what is the purpose of this. Indeed, it is troubling. If we look into include/ginkgo/core/stop/combined.hpp or cuda/base/executor.cpp, there the error is not thrown. It could be that in these cases (at least the copy) we explicitly did not want to throw an error for whatever reason, but mark that this behavior is not correct. I think nonetheless throwing should be fine from a preliminary look.

Edit: the more I think about it, the more I realize it was simply misused in the case the throw was not added, and technically it should be there.

@upsj AFAIK, the purpose of the testing two strategies in the conversions is to be future proof. If at some point we put classical CSR as the default of whatever, we ensure to not test a default. the load_balance strategy is already tested in cuda/test/matrix/csr_kernels.cpp, so I think we do not have to test this here on top of it?

pratikvn · 2019-11-13T10:15:25Z

Yes, I too think that changing the GKO_NOT_SUPPORTED to throw can technically not be considered "interface breaking" and should be added as that is indeed its expected behaviour.

yhmtsai

LGTM

thoasm · 2019-11-13T11:11:54Z

@upsj I agree with @tcojean: The strategy is just a shared_ptr that needs to be set properly. In the test, I use two different strategies to be sure that at least one is not randomly the default case (the default case could be changed later on).
The strategies should already be covered in the CUDA CSR test.

yhmtsai · 2019-11-13T11:15:48Z

To handle load_balance, automatical in hip and cuda, it creates a new strategy to get the new corresponding executor strategy in #373. Thus, it will change the strategy pointer.

upsj

LGTM

tcojean

LGTM. I think you could also move the throw into GKO_NOT_SUPPORTED because I believe it really should be there. Also, this is technically internal although due to using this in public hpp code we have to put this in the public space.

tcojean · 2019-11-13T13:59:02Z

include/ginkgo/core/preconditioner/ilu.hpp

@@ -290,7 +291,8 @@ class Ilu : public EnableLinOp<Ilu<LSolverType, USolverType, ReverseApply>> {
 SolverType,
 xstd::void_t<decltype(std::declval<factory_type_t<SolverType>>()
 .with_criteria(with_criteria_param_type()))>>
- : std::true_type {};
+ : std::true_type {
+ };


simple comment, you don't have to do anything: interesting clang format bug?

thoasm · 2019-11-13T14:32:36Z

Exacty that. Unfortunately, I was unable to undo that change. By the way, if there is a chance we can get this to xSDK when we merge it now, feel free to do that. Otherwise, I will merge it tonight when I am back at my PC.

pratikvn

LGTM!

include/ginkgo/core/base/exception_helpers.hpp

- All conversions to CSR (except CSR to CSR) now conserve the original strategy (test for that are included) - CSR strategies now try to copy the least amount of data - Add exception throwing in case the load_balance would not work properly (when srow is empty)

- Make strategy helper variables const to ensure they do not change - Fix Hybrid read function

- GKO_NOT_SUPPORTED is the only macro that does not include the `throw`. This was not only a problem in this code, but also in unrelated code, which is also fixed within this commit - An Empty Dense matrix can now be converted to a CSR matrix - Moved the `GKO_NOT_SUPPORTED` to the cuda kernel, so empty matrices do not throw when applying a strategy.

- Change a `static_cast` in the strategies to a constructor call - Move empty matrix test from CSR to Dense (and add a move test)

This version of Ginkgo provides a few fixes in Ginkgo's core routines. The supported systems and requirements are unchanged from version 1.1.0. ### Fixes + Improve Ginkgo's installation and fix the `test_install` step ([#406](#406)), + Fix some documentation issues ([#406](#406)), + Fix multiple code issues reported by sonarqube ([#406](#406)), + Update the git-cmake-format repository ([#399](#399)), + Improve the global update header script ([#390](#390)), + Fix broken bounds checks ([#388](#388)), + Fix CSR strategies and improve performance ([#379](#379)), + Fix a small typo in the stencil examples ([#381](#381)), + Fix ELL error on small matrices ([#375](#375)), + Fix SellP read function ([#374](#374)), + Add factorization support in `create_new_algorithm.sh` ([#371](#371)).

Minor release v1.1.1 This version of Ginkgo provides a few fixes in Ginkgo's core routines. The supported systems and requirements are unchanged from version 1.1.0. ### Fixes + Fix the `test_install` step with `HIP` ([#409](#409)), + Improve Ginkgo's installation and fix the `test_install` step ([#406](#406)), + Fix some documentation issues ([#406](#406)), + Fix multiple code issues reported by sonarqube ([#406](#406)), + Update the git-cmake-format repository ([#399](#399)), + Improve the global update header script ([#390](#390)), + Fix broken bounds checks ([#388](#388)), + Fix CSR strategies and improve performance ([#379](#379)), + Fix a small typo in the stencil examples ([#381](#381)), + Fix ELL error on small matrices ([#375](#375)), + Fix SellP read function ([#374](#374)), + Add factorization support in `create_new_algorithm.sh` ([#371](#371))

thoasm added is:enhancement An improvement of an existing feature. mod:core This is related to the core module. type:matrix-format This is related to the Matrix formats 1:ST:ready-for-review This PR is ready for review labels Nov 12, 2019

thoasm requested review from upsj, pratikvn, yhmtsai, hartwiganzt and tcojean November 12, 2019 20:24

thoasm self-assigned this Nov 12, 2019

pratikvn reviewed Nov 12, 2019

View reviewed changes

include/ginkgo/core/matrix/csr.hpp Outdated Show resolved Hide resolved

thoasm changed the title ~~Improved CSR strategies~~ Improve CSR strategies Nov 12, 2019

thoasm force-pushed the fix_csr_strategy branch from b98ee21 to b412b30 Compare November 12, 2019 21:07

tcojean previously approved these changes Nov 12, 2019

View reviewed changes

core/matrix/hybrid.cpp Show resolved Hide resolved

include/ginkgo/core/matrix/csr.hpp Outdated Show resolved Hide resolved

yhmtsai reviewed Nov 12, 2019

View reviewed changes

include/ginkgo/core/matrix/csr.hpp Outdated Show resolved Hide resolved

thoasm mentioned this pull request Nov 13, 2019

Make all conversions of empty matrices work #380

Closed

thoasm requested a review from tcojean November 13, 2019 03:38

upsj reviewed Nov 13, 2019

View reviewed changes

core/matrix/dense.cpp Show resolved Hide resolved

include/ginkgo/core/matrix/csr.hpp Outdated Show resolved Hide resolved

reference/test/matrix/csr_kernels.cpp Outdated Show resolved Hide resolved

reference/test/matrix/csr_kernels.cpp Outdated Show resolved Hide resolved

yhmtsai approved these changes Nov 13, 2019

View reviewed changes

upsj approved these changes Nov 13, 2019

View reviewed changes

tcojean approved these changes Nov 13, 2019

View reviewed changes

tcojean added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels Nov 13, 2019

tcojean reviewed Nov 13, 2019

View reviewed changes

pratikvn approved these changes Nov 13, 2019

View reviewed changes

thoasm force-pushed the fix_csr_strategy branch from 5667c30 to c0fface Compare November 13, 2019 15:51

upsj reviewed Nov 13, 2019

View reviewed changes

include/ginkgo/core/base/exception_helpers.hpp Outdated Show resolved Hide resolved

Thomas Grützmacher added 6 commits November 13, 2019 19:34

Improve CSR strategies

e524097

- All conversions to CSR (except CSR to CSR) now conserve the original strategy (test for that are included) - CSR strategies now try to copy the least amount of data - Add exception throwing in case the load_balance would not work properly (when srow is empty)

Review update

bd7f21f

- Make strategy helper variables const to ensure they do not change - Fix Hybrid read function

Review update

1974cd5

- Change a `static_cast` in the strategies to a constructor call - Move empty matrix test from CSR to Dense (and add a move test)

Fix GKO_NOT_SUPPORTED, so it behaves as expected

79242bb

Review update: fix doxygen comment for GKO_NOT_SUPPORTED

3663e0c

thoasm force-pushed the fix_csr_strategy branch from d4709d4 to 3663e0c Compare November 13, 2019 18:34

thoasm merged commit f4ae401 into develop Nov 14, 2019

thoasm deleted the fix_csr_strategy branch November 14, 2019 03:01

yhmtsai mentioned this pull request Nov 26, 2019

Possible interface breaking in Ginkgo 2.0.0 #402

Open

tcojean mentioned this pull request Nov 27, 2019

Minor Release v1.1.1 #406

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve CSR strategies #379

Improve CSR strategies #379

thoasm commented Nov 12, 2019 •

edited

Loading

tcojean left a comment

thoasm commented Nov 13, 2019

upsj left a comment

tcojean commented Nov 13, 2019 •

edited

Loading

pratikvn commented Nov 13, 2019

yhmtsai left a comment

thoasm commented Nov 13, 2019

yhmtsai commented Nov 13, 2019

upsj left a comment

tcojean left a comment

tcojean Nov 13, 2019

thoasm commented Nov 13, 2019 via email •

edited

Loading

pratikvn left a comment

Improve CSR strategies #379

Improve CSR strategies #379

Conversation

thoasm commented Nov 12, 2019 • edited Loading

tcojean left a comment

Choose a reason for hiding this comment

thoasm commented Nov 13, 2019

upsj left a comment

Choose a reason for hiding this comment

tcojean commented Nov 13, 2019 • edited Loading

pratikvn commented Nov 13, 2019

yhmtsai left a comment

Choose a reason for hiding this comment

thoasm commented Nov 13, 2019

yhmtsai commented Nov 13, 2019

upsj left a comment

Choose a reason for hiding this comment

tcojean left a comment

Choose a reason for hiding this comment

tcojean Nov 13, 2019

Choose a reason for hiding this comment

thoasm commented Nov 13, 2019 via email • edited Loading

pratikvn left a comment

Choose a reason for hiding this comment

thoasm commented Nov 12, 2019 •

edited

Loading

tcojean commented Nov 13, 2019 •

edited

Loading

thoasm commented Nov 13, 2019 via email •

edited

Loading