Skip to content

Simplify the testing types #1594

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
Merged
merged 7 commits into from
Oct 22, 2024
Merged

Simplify the testing types #1594

merged 7 commits into from
Oct 22, 2024

Conversation

@thoasm
Copy link
Member

@thoasm thoasm commented Apr 8, 2024 •
edited
Loading

Add template-type functions to combine and merge type lists. As a result, the GINKGO_DPCPP_SINGLE_MODE only needs to be present once.

Additionally, change the typen name ValueAndIndexType to ComplexAndPODTypes (because gko::size_type is not an IndexType).

TODO:

  • Add documentation to the type helper
  • go through all existing tests and use the wrapper in all cases we define types on our own (example: CB-Gmres)

@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. mod:core This is related to the core module. mod:cuda This is related to the CUDA module. mod:reference This is related to the reference module. labels Apr 8, 2024
@thoasm thoasm force-pushed the simplify_test_types branch 2 times, most recently from d648b9a to 5d94d98 Compare April 11, 2024 17:02
@thoasm thoasm requested review from a team April 11, 2024 17:03
@thoasm thoasm self-assigned this Apr 11, 2024
@thoasm thoasm marked this pull request as ready for review April 11, 2024 17:04
@thoasm thoasm added 1:ST:ready-for-review This PR is ready for review 1:ST:no-changelog-entry Skip the wiki check for changelog update labels Apr 11, 2024
@MarcelKoch MarcelKoch self-requested a review April 19, 2024 07:50
MarcelKoch
MarcelKoch requested changes Apr 19, 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.

Generally lgtm. The biggest issue for me is the documentation. It's dupliated a lot and should only be in one place (I think I would prefer the aliases in the gko::test namespace).

@MarcelKoch MarcelKoch added this to the Ginkgo 1.8.0 milestone Apr 19, 2024
@thoasm thoasm requested review from a team and MarcelKoch April 24, 2024 11:01
@MarcelKoch MarcelKoch removed this from the Ginkgo 1.8.0 milestone May 3, 2024
@tcojean tcojean added this to the Ginkgo 1.9.0 milestone May 3, 2024
@thoasm thoasm force-pushed the simplify_test_types branch 3 times, most recently from 869b2c4 to 795db27 Compare July 12, 2024 11:40
@thoasm thoasm force-pushed the simplify_test_types branch from 795db27 to 0b8b67c Compare October 18, 2024 09:20
yhmtsai
yhmtsai approved these changes Oct 18, 2024
View reviewed changes
Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

LGTM.
Because it is only used for testing, it is fine for generating std::tuple<> in cartesian product.
Just want to mention that I have also made some similar stuff for different purpose in first draft of file config https://github.com/ginkgo-project/ginkgo/blob/resource_manager/extension/resource_manager/include/resource_manager/base/type_pack.hpp with restricted and explicit outer_type and inner_type, so I do not need to distinguish after 2D caresian.

I think you can merge before half precision.
It might give some annoying rebase step for me, but it makes the half pr more clear.

@yhmtsai yhmtsai 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 Oct 21, 2024
Thomas Grützmacher added 2 commits October 21, 2024 17:27
@thoasm
Simplify the testing types
130560b 
Add template type functions to combine and merge type lists.
As a result, the GINKGO_DPCPP_SINGLE_MODE only needs to be present once.

Additionally, change the typen name ValueAndIndexType to ComplexAndPODTypes
(because gko::size_type is not an IndexType).
@thoasm
Use new type aliases in relevant tests
7cc9054 
Additionally, introduce `IntegerTypes`, which is IndexTypes combined
with gko::size_type (used for the prefix sum tests).
Thomas Grützmacher and others added 5 commits October 21, 2024 17:27
@thoasm thoasm force-pushed the simplify_test_types branch from 3b2b4a2 to a881a3d Compare October 21, 2024 15:29
@sonarqubecloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@codecov
Copy link

codecov bot commented Oct 22, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.25%. Comparing base (19269b6) to head (a881a3d).
Report is 8 commits behind head on develop.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #1594      +/-   ##
===========================================
+ Coverage    89.96%   90.25%   +0.28%     
===========================================
  Files          763      764       +1     
  Lines        62877    62907      +30     
===========================================
+ Hits         56570    56779     +209     
+ Misses        6307     6128     -179

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

@thoasm thoasm merged commit 7b04ae9 into develop Oct 22, 2024
12 of 14 checks passed
@thoasm thoasm deleted the simplify_test_types branch October 22, 2024 09:53
MarcelKoch pushed a commit to MarcelKoch/ginkgo that referenced this pull request Dec 2, 2024
@thoasm @MarcelKoch
Merge the simplification of testing types
8f1f6bc 
Add template-type functions to combine and merge type lists for tests.
As a result, the preprocessor check for GINKGO_DPCPP_SINGLE_MODE only needs to
be present once instead of 11 times.

Additionally, change the type name ValueAndIndexType to ComplexAndPODTypes
(because gko::size_type is not an IndexType).

Related PR: ginkgo-project#1594
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

@yhmtsai yhmtsai yhmtsai approved these changes

Assignees

@thoasm thoasm

Labels

1:ST:no-changelog-entry Skip the wiki check for changelog update 1:ST:ready-to-merge This PR is ready to merge. mod:core This is related to the core module. mod:cuda This is related to the CUDA module. mod:reference This is related to the reference module. reg:build This is related to the build system. reg:testing This is related to testing.

Projects

None yet

Milestone

Ginkgo 1.9.0

Development

Successfully merging this pull request may close these issues.

5 participants

@thoasm @MarcelKoch @yhmtsai @tcojean @ginkgo-bot