-
Notifications
You must be signed in to change notification settings - Fork 92
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
Release/1.2.0 #576
Release/1.2.0 #576
Conversation
The Ginkgo team is proud to announce the new minor release of Ginkgo version Supported systems and requirements:
The current known issues can be found in the known issues page. AdditionsHere are the main additions to the Ginkgo library. Other thematic additions are listed below.
Example additions
Compilation and library changes
Other additions
FixesAlgorithms
Other core functionalities
CUDA and HIP specific
Other
Tools and ecosystemBenchmarks
CI related
Test suite
Other
|
Note that for some reason, newer gtest versions do not work well with older CUDA versions: |
Codecov Report
@@ Coverage Diff @@
## develop #576 +/- ##
========================================
Coverage 84.22% 84.22%
========================================
Files 296 296
Lines 20653 20655 +2
========================================
+ Hits 17395 17397 +2
Misses 3258 3258
Continue to review full report at Codecov.
|
|
Regarding the sanitizers: We could probably get rid of all but the deliberate UBSAN warnings by adding a non-null/non-empty check to Array::operator= and all places where we call executor->copy() on empty data. |
And clang's libomp can be built with TSAN support enabled, should I look into setting up a container for that? |
The README.md still has the old link for the Contributor guidelines. Can you please update that ? |
With TSAN-enabled OpenMP runtime, the thread sanitizer run now also looks good. To get this to work, I needed the following steps:
The found issues are
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I think we can already add a section in README.md for citing the JOSS paper. openjournals/joss-reviews#2260. I think they already reserve the DOI, so we should be able to use that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this PR is not compiling with GCC 10.1, I would suggest to remove that from the list of supported compilers.
README.md
Outdated
@@ -37,7 +37,7 @@ For Ginkgo core library: | |||
|
|||
* _cmake 3.9+_ | |||
* C++11 compliant compiler, one of: | |||
* _gcc 5.3+, 6.3+, 7.3+, 8.1+_ | |||
* _gcc 5.3+, 6.3+, 7.3+, 8.1+, 9.1+, 10.1+_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure we are compatible with these? I tried it with GCC 9.1
some time ago, and it failed. The error looked very similar to what I get with GCC 10.1
now, which is described in this issue: #579
I think we should remove the last two additions until we resolve the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should also list the clang major versions we support, and remove the TODO from AppleClang?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clang, I don't think there really is any difference between the major releases in our case, since 3.9 and full C++11 support there has been no major problem. For GCC, the issue was that several major versions exist next to each other, and C++ 11 support was incomplete for example until 5.3 (at least, there were bugs with the compiler). I remember I did the work of fixing the issues we had with 5.3 some time ago, but going further down was too much work.
For AppleClang, as I have no MacOS available I cannot judge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point about clang. My main hold-up is that since we list versions explicitly for GCC, readers might assume that we only support a very old clang, but that is only a very vague issue.
We could list AppleClang 11, since that's what the Github Action uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this to say all versions after 8.1+
. I think this shows that the previous listings are an exception and otherwise we support all versions after the ones listed here.
@pratikvn I don't see a DOI for the JOSS paper? How can I find it. |
It has been reserved but not yet registered. https://github.com/openjournals/joss-papers/blob/joss.02260/joss.02260/10.21105.joss.02260.pdf . Essentially, the doi will be 10.21105/joss.02260. But as it has not yet been registered, we can also wait to add it. But this is will the final doi. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Here are some details on the Citation file format: https://citation-file-format.github.io/ |
Why use this tool and not just plain bibtex? Or markdown with a bibtex code portion (if it works). Is this integrated into github? Test: @misc{anzt2020ginkgo,
title={Ginkgo: A Modern Linear Operator Algebra Framework for High Performance Computing},
author={Hartwig Anzt and Terry Cojean and Goran Flegar and Fritz Göbel and Thomas Grützmacher and Pratik Nayak and Tobias Ribizel and Yuhsiang Mike Tsai and Enrique S. Quintana-Ortí},
year={2020},
eprint={2006.16852},
archivePrefix={arXiv},
primaryClass={cs.MS}
} |
I think the advantage is that it is machine readable. You can also add multiple references to the same citation file format file, for example all the papers that use Ginkgo. https://github.com/citation-file-format/citation-file-format/blob/master/README.md#references-optional. It is also in line with some existing standards: https://peerj.com/articles/cs-86/ |
Well, bibtex is also machine readable since there are several parsers and tools designed to use it directly. I think though this citation format is rather when you do not have a paper for your software but create a DOI for every release which can be cited. In our case though, I would rather not do that, or at least first point to the Interface paper and other topical papers (SpMV performance, HIP portability, ... for example), since otherwise citations get lost in many different references and you lose traction in google scholar and all other platforms. |
Okay. That makes sense. I guess we can add a section in the README on "How to cite Ginkgo" and add the bibTeX data as you mentioned in your comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it seems to be an ArchLinux problem since it works on Ubuntu, LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -0,0 +1,94 @@ | |||
# Citing Ginkgo {#citing_ginkgo} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is any usage of the {#citing_ginkgo}?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, see doc/DoxygenLayout.xml
Co-authored-by: Pratik Nayak <[email protected]> Co-authored-by: Tobias Ribizel <[email protected]>
If there is no issues with the recent changes, including the new |
I also used clang-format with version 8 on the full project so that everything is formatted according to this version as suggested by Mike. |
Maybe valid concerns about default reference captures in lambdas ? |
As the one who wrote that code, I would say we can safely ignore this. You can basically ready this function as two nested for-each loops that calls the first callback function for each row and the second callback function for each entry of the matrix sum. Reasons why it could make sense:
|
Co-authored-by: Pratik Nayak <[email protected]>
SonarCloud Quality Gate failed. 0 Bugs 80.0% Coverage The version of Java (1.8.0_121) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11. |
This PR is to push the 1.2.0 release into develop. I start with develop since it makes reviewing the changes easier.
The documentation should appear at this link after a successful build:
https://ginkgo-project.github.io/ginkgo/doc/release/1.2.0/
The various checks (sanitizers in particular) should be available here after a successful build:
https://my.cdash.org/index.php?project=Ginkgo+Project&date=2020-06-23
The next post will have a proposition for a changelog. Feel free to update it or propose improvements.