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.

Sign up for GitHub

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

Jump to bottom

Remove templated matrix constructors #1433

Merged
merged 33 commits into from
Feb 19, 2024
Merged

Remove templated matrix constructors #1433

merged 33 commits into from
Feb 19, 2024

Conversation

upsj
Copy link
Member

@upsj upsj commented Oct 15, 2023 •
edited
Loading

This removes all templated array constructors and replaces them by versions that are based on array conversion constructors. This doesn't have any performance impacts except for the case when we pass in an lvalue (non-temporary/moved) that lives on another executor than the object we are constructing, where it leads to an additional allocation and copy on the source executor. That seems like an unimportant case to me, so I'm fine with the change making the interface simpler.

One thing this does have to change is the initialization from initializer_list. I'm honestly not sure whether this was explicitly designed to work, accidentally worked or something in between, I definitely had to dig why it works (the initializer list is being forwarded to the array(exec, init_list) constructor. If it was intended, we can't really change it, but it seems more like of a coincidental thing to me?

EDIT: I dug some more, this was introduced in fcd8eff, where it seems intentionally intended for arrays only, and first used with initializer_list in #659, where it didn't come up in the discussion, so this kind of underlines the coincidental thing to me.

@upsj upsj added the 1:ST:ready-for-review This PR is ready for review label Oct 15, 2023
@upsj upsj requested a review from a team October 15, 2023 18:50
@upsj upsj self-assigned this Oct 15, 2023
@ginkgo-bot ginkgo-bot added reg:testing This is related to testing. mod:core This is related to the core module. mod:reference This is related to the reference module. type:preconditioner This is related to the preconditioners type:matrix-format This is related to the Matrix formats type:factorization This is related to the Factorizations type:multigrid This is related to multigrid labels Oct 15, 2023
@sonarcloud
Copy link

sonarcloud bot commented Oct 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.3) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@codecov
Copy link

codecov bot commented Oct 16, 2023 •
edited
Loading

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (fb70ec0) 91.04% compared to head (afd3e06) 91.22%.
Report is 103 commits behind head on develop.

❗ Current head afd3e06 differs from pull request most recent head 68e0022. Consider uploading reports for the commit 68e0022 to get more accurate results

Files Patch % Lines
reference/test/base/utils.cpp 75.00% 2 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #1433      +/-   ##
===========================================
+ Coverage    91.04%   91.22%   +0.17%     
===========================================
  Files          700      644      -56     
  Lines        56996    52904    -4092     
===========================================
- Hits         51894    48260    -3634     
+ Misses        5102     4644     -458

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@upsj upsj mentioned this pull request Oct 24, 2023
Merged
5 tasks
@MarcelKoch MarcelKoch self-requested a review January 24, 2024 10:16
MarcelKoch
MarcelKoch reviewed Jan 24, 2024
View reviewed changes
Copy link
Member

@MarcelKoch MarcelKoch left a comment

Choose a reason for hiding this comment

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

first feedback. I would suggest to deprecate the initializer_list constructors. Also, maybe we should try to merge constructors by using invalid values as default values.

include/ginkgo/core/matrix/coo.hpp Show resolved Hide resolved
include/ginkgo/core/matrix/csr.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/matrix/csr.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/matrix/csr.hpp Show resolved Hide resolved
include/ginkgo/core/matrix/csr.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/matrix/hybrid.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/matrix/identity.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/matrix/permutation.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/matrix/row_gatherer.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/distributed/partition.hpp Show resolved Hide resolved
@upsj
Copy link
Member Author

upsj commented Jan 24, 2024 •
edited
Loading

@MarcelKoch Indeed the duplicate constructors are due to the fact that default parameters can't depend on other parameters. But the nullptr workaround sounds like a nice solution!

@upsj upsj requested a review from MarcelKoch February 5, 2024 15:53
@thoasm thoasm self-requested a review February 7, 2024 09:50
@upsj upsj requested a review from nbeams February 7, 2024 09:51
@tcojean tcojean self-requested a review February 7, 2024 09:51
MarcelKoch
MarcelKoch approved these changes Feb 9, 2024
View reviewed changes
Copy link
Member

@MarcelKoch MarcelKoch left a comment

Choose a reason for hiding this comment

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

LGTM, only some minior comments.

include/ginkgo/core/matrix/dense.hpp Show resolved Hide resolved
include/ginkgo/core/matrix/ell.hpp Show resolved Hide resolved
core/matrix/batch_identity.cpp Show resolved Hide resolved
@nbeams
Copy link
Collaborator

nbeams commented Feb 13, 2024

As a note, I tried this branch with MFEM and can confirm that it doesn't seem to affect the integration.

thoasm
thoasm approved these changes Feb 19, 2024
View reviewed changes
Copy link
Member

@thoasm thoasm left a comment

Choose a reason for hiding this comment

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

LGTM!

@upsj upsj 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 Feb 19, 2024
upsj and others added 24 commits February 19, 2024 11:03
@upsj
fbcsr
f80b03b
@upsj
hybrid
867a4d1
@upsj
row_gatherer
7588230
@upsj
sellp
a928623
@upsj
sparsity_csr
bf1ac0d
@upsj
scaled_permutation
b4dcab4
@upsj
fbcsr
387f600
@upsj
batch_csr
b8741cf
@upsj
ell
ac6c398
@upsj
logger docs
604fbd6
@upsj
batch_ell
d05878c
@upsj
fft
df893f5
@upsj
batch_dense
9ca4cfe
@upsj
identity
0187f04
@upsj
batch_identity
442bc3f
@upsj
permutation
d4b8384
@upsj
vector
6643c86
@upsj
remove partition create
5d94f0c
@upsj
matrix
024b3da
@upsj
partition
5b7d8d6
@upsj
identity
5bab4d6
@upsj
perturbation
3ed7e3c
@upsj @MarcelKoch
review updates
7950e19 
- remove redundant constructors using default arguments
  Now most matrix classes only have two constructors:
  One with sizes for uninitialized data,
  and one with arrays for initialized data
- add missing return value documentation
- add missing documentation

Co-authored-by: Marcel Koch <[email protected]>
@upsj
deprecate initializer_list constructors
7ecfbce
@upsj @MarcelKoch
review updates
68e0022 
- Add comments for stride = 0 default value
- Require square size for batch identity matrix

Co-authored-by: Marcel Koch <[email protected]>
@upsj upsj merged commit ba16a9b into develop Feb 19, 2024
12 of 15 checks passed
@upsj upsj deleted the array_constructors branch February 19, 2024 14:57
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Feb 20, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarcelKoch MarcelKoch MarcelKoch approved these changes

@thoasm thoasm thoasm approved these changes

@nbeams nbeams Awaiting requested review from nbeams

@tcojean tcojean Awaiting requested review from tcojean

Assignees

@upsj upsj

Labels
1:ST:ready-to-merge This PR is ready to merge. mod:core This is related to the core module. mod:reference This is related to the reference module. reg:testing This is related to testing. type:factorization This is related to the Factorizations type:matrix-format This is related to the Matrix formats type:multigrid This is related to multigrid type:preconditioner This is related to the preconditioners
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@upsj @nbeams @MarcelKoch @thoasm @ginkgo-bot