Improve naming of common member functions #1400

upsj · 2023-08-29T13:15:58Z

Applies the following renamings:

array::get_num_elems() to get_size()
matrix_data::ensure_row_major_order() to sort_row_major() since ensure doesn't really make clear whether this is a predicate/check or an action.
device_matrix_data::get_num_elems() to device_matrix_data::get_num_stored_elements()

include/ginkgo/core/base/array.hpp

MarcelKoch

I like the changes, especially get_num_elems was a bit lengthy. The deprecation warning in array still has to be fixed.

stale

MarcelKoch

I only checked if the old versions are called anywhere. Since it compiles and passes tests, I'm assuming that all instance have been correctly replaced.

include/ginkgo/core/base/matrix_data.hpp

sonarqubecloud · 2023-09-22T00:35:29Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
18 Code Smells

86.1% Coverage
15.9% Duplication

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

MarcelKoch

I think the renaming in device_matrix_data and index_set are not appropriate.

include/ginkgo/core/base/device_matrix_data.hpp

include/ginkgo/core/base/index_set.hpp

nbeams · 2023-09-22T13:55:13Z

I would like to point out that these changes will affect the MFEM integration (use of get_const_data); not sure about any other integrations. For MFEM, it would be simple to update, but could this PR be merged close to a tag or minor release if possible, to make it easier to check the Ginkgo version to decide which version to use?

upsj · 2023-12-02T20:53:29Z

format!

sonarqubecloud · 2023-12-04T05:31:18Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

81.6% Coverage
5.0% Duplication

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 · 2023-12-04T05:39:06Z

Codecov Report

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

Comparison is base (aa5e26d) 89.30% compared to head (a460594) 91.33%.

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

Files	Patch %	Lines
include/ginkgo/core/base/device_matrix_data.hpp	25.00%	3 Missing ⚠️
include/ginkgo/core/matrix/csr.hpp	66.66%	3 Missing ⚠️
include/ginkgo/core/base/batch_multi_vector.hpp	0.00%	2 Missing ⚠️
include/ginkgo/core/matrix/batch_ell.hpp	33.33%	2 Missing ⚠️
include/ginkgo/core/matrix/coo.hpp	33.33%	2 Missing ⚠️
include/ginkgo/core/matrix/fbcsr.hpp	50.00%	2 Missing ⚠️
omp/matrix/fbcsr_kernels.cpp	0.00%	2 Missing ⚠️
common/unified/matrix/hybrid_kernels.cpp	50.00%	1 Missing ⚠️
core/matrix/permutation.cpp	0.00%	1 Missing ⚠️
include/ginkgo/core/base/array.hpp	91.66%	1 Missing ⚠️
... and 8 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1400      +/-   ##
===========================================
+ Coverage    89.30%   91.33%   +2.03%     
===========================================
  Files          688      688              
  Lines        56338    56089     -249     
===========================================
+ Hits         50311    51228     +917     
+ Misses        6027     4861    -1166

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

pratikvn

LGTM! I think with the get_ prefix and only for array, this makes a lot more sense. Should we also replace the internal member of array num_elems_ to size_ as well ?

include/ginkgo/core/base/array.hpp

yhmtsai

code LGTM. but I would like to ensure the others are okay with this naming change

yhmtsai · 2023-12-04T09:23:37Z

examples/kokkos_assembly/kokkos_assembly.cpp

 #include <iostream>
+#include <Kokkos_Core.hpp>
 #include <map>
 #include <string>


 #include <omp.h>
+
+
 #include <ginkgo/ginkgo.hpp>
-#include <Kokkos_Core.hpp>


should Kokkos_Core be in the third party section?

I think we can handle that with #1358?

include/ginkgo/core/base/matrix_data.hpp

test/base/device_matrix_data_kernels.cpp

- rename num_elems_ members - use this-> in matrix_data - fix incorrect test Co-authored-by: Yuhsiang M. Tsai <[email protected]> Co-authored-by: Pratik Nayak <[email protected]>

thoasm

LGTM!

include/ginkgo/core/base/array.hpp

Fixing the array size member name after #1400 changed it. Related PR: #1500

upsj added the 1:ST:ready-for-review This PR is ready for review label Aug 29, 2023

upsj self-assigned this Aug 29, 2023

MarcelKoch reviewed Aug 30, 2023

View reviewed changes

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

MarcelKoch previously approved these changes Sep 11, 2023

View reviewed changes

thoasm self-requested a review September 18, 2023 12:33

upsj force-pushed the simplify_array branch from 7bc8285 to b373c77 Compare September 19, 2023 09:01

upsj changed the title ~~Rename array members to match standard library~~ Improve naming of common member functions Sep 19, 2023

upsj requested a review from MarcelKoch September 19, 2023 09:07

MarcelKoch requested changes Sep 20, 2023

View reviewed changes

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

upsj force-pushed the simplify_array branch from b373c77 to a0ea878 Compare September 21, 2023 13:47

upsj requested a review from MarcelKoch September 21, 2023 14:17

MarcelKoch requested changes Sep 22, 2023

View reviewed changes

include/ginkgo/core/base/device_matrix_data.hpp Show resolved Hide resolved

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

upsj force-pushed the simplify_array branch from a0ea878 to e7e7bb4 Compare September 23, 2023 10:44

tcojean mentioned this pull request Oct 11, 2023

Add a Dense batched class and kernels #1413

Merged

3 tasks

upsj force-pushed the simplify_array branch from e7e7bb4 to d9d1c22 Compare November 21, 2023 12:54

upsj requested a review from MarcelKoch November 21, 2023 12:56

upsj requested a review from MarcelKoch December 2, 2023 20:49

ginkgo-project deleted a comment from github-actions bot Dec 2, 2023

upsj mentioned this pull request Dec 2, 2023

Clearer index_set member names #1484

Open

upsj added the 1:ST:run-full-test label Dec 2, 2023

upsj force-pushed the simplify_array branch from 5043be7 to 53e64cd Compare December 2, 2023 20:59

upsj added 6 commits December 3, 2023 21:22

rename ensure_row_major_order to sort_row_major

dd3e4b2

rename array::get_num_elems() to get_size()

4e859d2

fix residual norm warnings

d5d93b7

fixes and rename device_matrix_data::get_num_elems

9dc2f68

fix bugs

ae9ae2a

fix some warnings

ded93c3

upsj force-pushed the simplify_array branch from 53e64cd to ded93c3 Compare December 3, 2023 20:37

formatting

a460594

MarcelKoch approved these changes Dec 4, 2023

View reviewed changes

pratikvn approved these changes Dec 4, 2023

View reviewed changes

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

Disable windows-mingw

5d6f996

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 1:ST:run-full-test labels Dec 4, 2023

yhmtsai reviewed Dec 4, 2023

View reviewed changes

review updates

befc7dc

- rename num_elems_ members - use this-> in matrix_data - fix incorrect test Co-authored-by: Yuhsiang M. Tsai <[email protected]> Co-authored-by: Pratik Nayak <[email protected]>

thoasm approved these changes Dec 4, 2023

View reviewed changes

yhmtsai approved these changes Dec 4, 2023

View reviewed changes

include/ginkgo/core/base/array.hpp Show resolved Hide resolved

upsj merged commit 37a63de into develop Dec 4, 2023
12 of 15 checks passed

upsj deleted the simplify_array branch December 4, 2023 13:36

upsj added a commit that referenced this pull request Dec 7, 2023

Merge updated gdb pretty-printer

c67dc10

Fixing the array size member name after #1400 changed it. Related PR: #1500

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve naming of common member functions #1400

Improve naming of common member functions #1400

upsj commented Aug 29, 2023 •

edited

Loading

MarcelKoch left a comment

MarcelKoch left a comment •

edited

Loading

sonarqubecloud bot commented Sep 22, 2023

MarcelKoch left a comment

nbeams commented Sep 22, 2023

upsj commented Dec 2, 2023

sonarqubecloud bot commented Dec 4, 2023

codecov bot commented Dec 4, 2023 •

edited

Loading

pratikvn left a comment

yhmtsai left a comment

yhmtsai Dec 4, 2023

upsj Dec 4, 2023

thoasm left a comment

Improve naming of common member functions #1400

Improve naming of common member functions #1400

Conversation

upsj commented Aug 29, 2023 • edited Loading

MarcelKoch left a comment

Choose a reason for hiding this comment

MarcelKoch left a comment • edited Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 22, 2023

MarcelKoch left a comment

Choose a reason for hiding this comment

nbeams commented Sep 22, 2023

upsj commented Dec 2, 2023

sonarqubecloud bot commented Dec 4, 2023

codecov bot commented Dec 4, 2023 • edited Loading

Codecov Report

pratikvn left a comment

Choose a reason for hiding this comment

yhmtsai left a comment

Choose a reason for hiding this comment

yhmtsai Dec 4, 2023

Choose a reason for hiding this comment

upsj Dec 4, 2023

Choose a reason for hiding this comment

thoasm left a comment

Choose a reason for hiding this comment

upsj commented Aug 29, 2023 •

edited

Loading

MarcelKoch left a comment •

edited

Loading

codecov bot commented Dec 4, 2023 •

edited

Loading