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.

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

Prettify README #1504

Merged
merged 3 commits into from
Dec 12, 2023
Merged

Prettify README #1504

merged 3 commits into from
Dec 12, 2023

Conversation

pratikvn
Copy link
Member

@pratikvn pratikvn commented Dec 10, 2023

This PR makes the README a bit more pretty and adds a TOC, which I think should improve readability.

See how it looks by going to the branch README file

@pratikvn pratikvn added reg:documentation This is related to documentation. 1:ST:ready-for-review This PR is ready for review 1:ST:skip-full-test 1:ST:no-changelog-entry Skip the wiki check for changelog update labels Dec 10, 2023
@pratikvn pratikvn requested a review from a team December 10, 2023 17:04
@pratikvn pratikvn self-assigned this Dec 10, 2023
@pratikvn pratikvn force-pushed the update-readme branch 2 times, most recently from 4791994 to 7dfe9de Compare December 10, 2023 17:08
Copy link

codecov bot commented Dec 10, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6adf1a6) 89.37% compared to head (7dfe9de) 89.33%.
Report is 2 commits behind head on develop.

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

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1504      +/-   ##
===========================================
- Coverage    89.37%   89.33%   -0.05%     
===========================================
  Files          696      688       -8     
  Lines        56944    56557     -387     
===========================================
- Hits         50895    50525     -370     
+ Misses        6049     6032      -17     

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

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@pratikvn pratikvn force-pushed the update-readme branch 5 times, most recently from 21ed856 to e67bc0b Compare December 12, 2023 07:44
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.

nit on the badge margin but it is not critical.

@@ -73,7 +75,7 @@ The Ginkgo DPC++(SYCL) module has the following __additional__ requirements:

The Ginkgo MPI module has the following __additional__ requirements:

* MPI 3.1+, ideally with GPUDirect support for best performance
* MPI 3.1+, ideally GPU-Aware, for best performance
Copy link
Member

Choose a reason for hiding this comment

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

I think GPU-Aware is different from GPUDirect.
GPU-Aware only recognizes whether the memory is on GPU such that we do not need to move the data to host.
GPU direct is more relevant for performance

Copy link
Member Author

Choose a reason for hiding this comment

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

No, GPUDirect is a NVIDIA related terminology that is unrelated to MPI. The correct term for MPI implementation is GPU-Aware is used across all MPI implementations. See for example paper from the MVAPICH2 team

Copy link
Member

Choose a reason for hiding this comment

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

GPU-Aware means that the MPI implementation is able to accept GPU pointers, it does not say whether they actually use direct connections between GPUs. That is what GPUDirect refers to on the NVIDIA side, I'm not sure if there is an equivalent for AMD or Intel.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@pratikvn pratikvn 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 Dec 12, 2023
pratikvn and others added 3 commits December 12, 2023 15:09
Co-authored-by: Marcel Koch <[email protected]>
Co-authored-by: Yu-Hsiang Tsai <[email protected]>
Co-authored-by: Gregor Olenik <[email protected]>
@pratikvn pratikvn merged commit a7a8dab into develop Dec 12, 2023
13 of 15 checks passed
@pratikvn pratikvn deleted the update-readme branch December 12, 2023 14:11
@ginkgo-bot
Copy link
Member

Error: PR already merged!

Copy link

sonarcloud bot commented Dec 13, 2023

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
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
Labels
1:ST:no-changelog-entry Skip the wiki check for changelog update 1:ST:ready-to-merge This PR is ready to merge. 1:ST:skip-full-test reg:documentation This is related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants