Closed editorialbot closed 1 year ago
editorialbot
commented
1 year ago Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.
For a list of things I can do to help you, just type:
@editorialbot commandsFor example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@editorialbot generate pdfSoftware report:
github.com/AlDanial/cloc v 1.88 T=0.88 s (1438.2 files/s, 217985.6 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
C++ 549 12619 17667 65906
C/C++ Header 498 8231 13555 63728
Markdown 51 699 0 4095
C 12 171 152 1723
CMake 123 345 422 1603
Bourne Shell 14 22 40 157
TeX 1 29 12 124
YAML 6 18 65 79
Dockerfile 1 14 34 42
JSON 9 0 0 27
-------------------------------------------------------------------------------
SUM: 1264 22148 31947 137484
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repositoryWordcount for paper.md is 1165
editorialbot
commented
1 year ago Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.23919/date.2019.8715262 is OK
- 10.1137/17m1140819 is OK
- 10.1109/mm.2021.3061394 is OK
- 10.1007/978-3-031-09779-9_2 is OK
- 10.1109/sc.2018.00050 is OK
- 10.1145/3148226.3148237 is OK
- 10.1007/978-3-319-93698-7_45 is OK
- 10.1137/18m1229511 is OK
- 10.1007/978-3-031-09779-9_7 is OK
MISSING DOIs
- None
INVALID DOIs
- None:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
danielskatz
commented
1 year ago @mmoelle1 and @mlxd - Thanks for agreeing to review this submission. This is the review thread for the paper. All of our communications will happen here from now on.
As you can see above, you each should use the command @editorialbot generate my checklist to create your review checklist. @editorialbot commands need to be the first thing in a new comment.
As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.
The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#5072 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.
We aim for reviews to be completed within about 2-4 weeks. Please let me know if either of you require some more time. We can also use editorialbot (our bot) to set automatic reminders if you know you'll be away for a known period of time.
Please feel free to ping me (@danielskatz) if you have any questions/concerns.
Ravenwater
commented
1 year ago @danielskatz do I need to worry about this? What is prohibiting gitinspector from succeeding?
gitinspector failed to run statistical information for the repositoryyou can ignore it
mmoelle1
commented
1 year ago My apologize but where should I run the command
@editorialbot generate my checklist
I never reviewed for JOSS before.
On 12 Jan 2023, at 15:35, Daniel S. Katz @.***> wrote:
you can ignore it
— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https://github.com/openjournals/joss-reviews/issues/5072*issuecomment-1380455438__;Iw!!PAKc-5URQlI!-haO3e5yMw0aIgFiJFay7kwLNGvXN1fSLCtwcb0a0rvjWgVhXpryKCpu-G0g2Y5gjeQs5RU5YH_9mRk_maIpCXpP9ns$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AAPGPI2Z7H5GH6FDXZUBRUTWSAJCFANCNFSM6AAAAAATZJETAM__;!!PAKc-5URQlI!-haO3e5yMw0aIgFiJFay7kwLNGvXN1fSLCtwcb0a0rvjWgVhXpryKCpu-G0g2Y5gjeQs5RU5YH_9mRk_maIp3_d9SYs$. You are receiving this because you were mentioned.Message ID: @.***>
danielskatz
commented
1 year ago I think you need to do this in directly in the GitHub web interface in this issue. (I don't think this will work by email but you can try it if you want.) An editorialbot command should be the first thing in a new comment in an issue. And I'm fairly sure you will need to check items in the checklist in the GitHub web interface as well.
mmoelle1
commented
1 year ago mmoelle1
commented
1 year ago I think you need to do this in directly in the GitHub web interface in this issue. (I don't think this will work by email but you can try it if you want.) An editorialbot command should be the first thing in a new comment in an issue. And I'm fairly sure you will need to check items in the checklist in the GitHub web interface as well.
Got it and it works.
mlxd
commented
1 year ago Ravenwater
commented
1 year ago @mmoelle1 @mlxd Thanks for reviewing this submission.
danielskatz
commented
1 year ago 👋 @yboettcher - thanks for agreeing to also review this submission
danielskatz
commented
1 year ago @editorialbot add @yboettcher as reviewer
editorialbot
commented
1 year ago @yboettcher added to the reviewers list!
danielskatz
commented
1 year ago @yboettcher - Thanks for agreeing to review this submission. This is the review thread for the paper. All of our communications will happen here from now on.
As you can see above, you each should use the command @editorialbot generate my checklist to create your review checklist. @editorialbot commands need to be the first thing in a new comment.
As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.
The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#5072 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.
We aim for reviews to be completed within about 2-4 weeks. Please let me know if either of you require some more time. We can also use editorialbot (our bot) to set automatic reminders if you know you'll be away for a known period of time.
Please feel free to ping me (@danielskatz) if you have any questions/concerns.
yboettcher
commented
1 year ago Ravenwater
commented
1 year ago @yboettcher thank you for reviewing this submission.
Ravenwater
commented
1 year ago I just release v3.67. It adds a 2-base logarithmic number system to Universal with some comparisons between small cfloats, lns, and lns2b, including the different proposed IEEE-754 FP8 formats for Deep Learning.
danielskatz
commented
1 year ago @editorialbot set v3.67 as version
editorialbot
commented
1 year ago Done! version is now v3.67
danielskatz
commented
1 year ago 👋 @mmoelle1 & @mlxd & @yboettcher - I'm just checking in with you all here, since we're a couple of weeks into the process. Do you have any questions or problems that I can help with?
mlxd
commented
1 year ago @danielskatz All good here, thanks. Will continue with the review over the coming days 👍
yboettcher
commented
1 year ago A few notes from my side, some of which are most likely fine the way they are, but I would still like to clarify these.
The first is in regards to the license and Contributing points. The license states that Stillwater Supercomputing holds the copyright to universal. I would assume that if someone were to make a code contribution to universal, this would mean that they transfer any (copy)rights they have to Stillwater. I am not an expert on legal things, but does the existence of a LICENSE file with a copyright holder imply this (in which case I would check off these items), or would it be better to explicitely mention this either in the Readme or a separate Contribution.md file?
The second one is about the authors: like previous publications on this library, the paper lists Theodore Omtzigt and James Quinlan as authors. However, judging the Contributors.md file, there are some more people who have worked on this. I would assume (and like to clarify) that authors refers to the "owners/driving forces" behind the software project, in which case the current list of authors looks sensible to me.
Some nitpicks on the software paper:
In the example at the end of the summary, the "color coded" form is not color coded as it is in the terminal. Seeing that the paper is generated from markdown and it doesn't seem like markdown supports coloring easily, I guess there is nothing to do about this.
The sentences on line 37 and 38 seem a bit strange to me. On l.37, the "environmental impacts and financial costs" are attributed to a single cause. On l.38 it says "these both result from". Due to the use of the plural, it appears to be referencing the "environmental impacts and financial costs" again and attributes them to a different cause. Continuing on l.38, I'm a bit confused about the "growth". I feel like this should say "growing requirements".
On line 43, there appears to be a broken reference (kharya:2020).
In both the paper and the github readme, you state that several projects "leverage" universal/posits. I feel like this is a little suboptimal wording. Out of the mentioned projects, I could only find references to universal in G+SMO, whereas "leverage" suggests (to me) that there is some integration of universal (or even a dependency on universal) into these projects, which I feel is not the case for mtl4, odeint and bembel.
And two notes on the readme:
Right at the beginning of the "Motivation" section, "the 64-bit format" is referenced, which might be a little ambiguous in the context of a library implementing many formats.
The make posit_32bit_posit command in the "Verification Suite" section does not work (for me atleast, no rule to for posit_32bit_posit). I suspect that this might just need updating of the target name.
jamesquinlan
commented
1 year ago @yboettcher thank you for the feedback. I will defer to @Ravenwater for many of these points, and I'll address what I think I can.
In the example, the color came through in markdown using cpp, but the output from the terminal is just plain markdown. But I note that in the same example, "some" colors, like a red left parenthesis and an olive green negative number. I'm not sure how to fix that (or if it can be).
L38+39. I agree. What we were trying to say comes from haidar et al. 2018 "...the increasing performance of HPC systems comes with a cost: electrical power consumption. This leads to the following two main concerns, an increase of the power bills beyond affordable budgets and an increasing impact on the environment." This is directly related to processing (and storage). I will rewrite that thought better. I also agree, "growing requirements" is more clear.
It looks like kharya:2020 is missing the @ in front to form the link to reference.
Thanks again for the review and feedback.
Ravenwater
commented
1 year ago @yboettcher The copyright holder is the person or entity that can claim the right to have created a piece of work, or to whom such right has been transferred. The MIT license is not about transferring copyright, but about granting the legal right to use the (copyrighted) work as stated in the license. If you write a class or function or application, you have the copyright to it, even if its actual use depends on third-party work, such as the Universal build and test system, if you include it into the Universal repo.
Once you add your contribution to the software package and distribute the whole package, the package has multiple copyright holders: the original authors, and you. You are allowed to distribute the parts from the original package because the MIT license allows you to "use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software".
Ravenwater
commented
1 year ago @yboettcher @jamesquinlan we need to make it clearer that if you want to build a specific target, then you would need to have enabled that build target in the build system. So if you want to build a posit target, such as make posit_32bit_posit, you would need to have executed a cmake -DBUILD_NUMBER_POSITS=ON .. to enable that build target.
Ravenwater
commented
1 year ago @yboettcher In the ArXiv paper: https://arxiv.org/ftp/arxiv/papers/2012/2012.11011.pdf we present mixed-precision results using Universal to extend ODEint, and the fused dot product is being enabled automatically in MTL4 when a posit scalar type is detected. And @yboettcher leveraged the posits of Universal to extend Bembel to generate new results.
So in all three cases, these platforms, which were written to be specialized with libraries like Universal, leverage Universal to create new results.
jamesquinlan
commented
1 year ago @Ravenwater, I agree. We can add that to the article's body and the readme file. I would also like to add a sentence on the new additions of v3.67 in the Summary.
As for the sentence in the statement of need, a slight modification should clarify: ``High-performance computing (HPC), machine learning, and deep learning tasks [e.g., @ carmichael:2019; @ cococcioni2022small;@ desrentes:2022posit8] have increased environmental impacts and financial costs due to massive energy consumption [@ haidar:2018b] from the growing processing requirements.
yboettcher
commented
1 year ago @Ravenwater Thanks for clarifying the copyright thing.
As for the make posit_32bit_posit, the following chain of commands returns an error (make: *** No rule to make target 'posit_32bit_posit'. Stop.):
git clone https://github.com/stillwater-sc/universal
cd universal/
mkdir build
cd build
cmake -DBUILD_NUMBER_POSITS=ON ..
make posit_32bit_positI can run make posit just fine though, so I guess posit_32bit_posit is an outdated target name.
It also seems like the list of provided number systems in the readme (section "Structure of the tree") might need updating.
Out of those, the adaptiveint/adaptivefloat/adaptiveposit don't exist in include/universal.
The quire only contains an exception.hpp, the unum2 does not contain a "main" unum2.hpp and the float also seems to be missing content.
On the other hand, you mention SORNs in the software paper, but not in the readme.
I also think it would be helpful if there was some way of knowing how to use the different number system templates. You have an example that shows how that works for posits, but as for the lns numbers (for example), I would not know, how many template parameters there are, or what they mean.
jamesquinlan
commented
1 year ago @yboettcher We could add more example templates to the paper, but probably readme best. Not sure if the length would be an issue if in the paper as I remember word ranges from https://joss.readthedocs.io/en/latest/submitting.html?highlight=length#what-should-my-paper-contain
Ravenwater
commented
1 year ago @yboettcher I have incorporated your suggestions and added several presentations we have done through the years explaining the number systems contained. I also fixed all the bugs and misinformation you highlighted. This is all done in the v3.68 branch, please take a look and let us know if that satisfies your concerns.
mlxd
commented
1 year ago Hi all, I have only a single item to add in addition to the comments from @yboettcher above:
I have added the above issue for item Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)? https://github.com/stillwater-sc/universal/issues/325
I think some updates would be appreciated here, especially around visibility of function/modules, and guiding the developer accordingly with given examples.
Ravenwater
commented
1 year ago @mlxd We Need to figure out how best to provide documentation. The types in Universal are plug-in arithmetic types, so for regular use, there is no API as you would use the values in arithmetic expressions just like you would use a native float or int.
However, as @yboettcher also identified, documentation to bring folks up to speed with arithmetic types Universal style would benefit from a description of how the collective was put together. That is more of a 'how-to-parameterize' arithmetic types how-to than API documentation, but would definitely be useful.
Then there is the whole verification environment and its design that does center around a common pattern to have the tests enumerate and drive raw bit patterns in the type so that the test framework can be generalized. If you look at the code allocation in Universal you'll see that this is where the bulk of the code is, it is not in the arithmetic types.
Each type template has documentation of what the parameters mean. Do we want to bring this up in a documentation file?
Ravenwater
commented
1 year ago @mlxd I just checked in a new markup file called docs/number-system-type-parameterization.md that describes in a one pager how the core types are parameterized. This is in the v3.68 branch. Love to get your quick feedback if that satisfies your request.
In each number system type's regression suite there is always an api/api.cpp that chronicles all the invokation and use cases to provide an executable example of how to use the type. In general, the api section of the regression tests has code examples how to use the different library components, such as manipulators, attributes, number traits, exceptions, and special cases. Furthermore, iIn the education build target, there are individual test programs that demonstrate how to use the different types. The documentation might need to high light that these resources are available.
Ravenwater
commented
1 year ago @mlxd @yboettcher @mmoelle1 I have updated the README.md to add extra sections that explain how to use and how to extend the library. This is in the v3.68 branch.
Love to hear your feedback if these additions alleviate your concerns.
jamesquinlan
commented
1 year ago @editorialbot set v3.68 as version
editorialbot
commented
1 year ago I'm sorry @jamesquinlan, I'm afraid I can't do that. That's something only editors are allowed to do.
jamesquinlan
commented
1 year ago @danielskatz please set to v3.68. Afterwards, @editorialbot generate pdf
danielskatz
commented
1 year ago @editorialbot set v3.68 as version
editorialbot
commented
1 year ago Done! version is now v3.68
danielskatz
commented
1 year ago @editorialbot generate pdf
editorialbot
commented
1 year ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
jamesquinlan
commented
1 year ago @danielskatz quick question, does setting to v3.68 read out of the v3.68 branch in the repo? The article proof doesn't contain the revisions, e.g., the missing reference.
Thanks again for your help.
danielskatz
commented
1 year ago No, this just sets the version tag. Changing the branch is a different command
danielskatz
commented
1 year ago @editorialbot set v3.68 as branch
editorialbot
commented
1 year ago Done! branch is now v3.68
danielskatz
commented
1 year ago @editorialbot generate pdf
Submitting author: !--author-handle-->@Ravenwater<!--end-author-handle-- (E. Theodore L. Omtzigt) Repository: https://github.com/stillwater-sc/universal Branch with paper.md (empty if default branch): main Version: 3.68 Editor: !--editor-->@danielskatz<!--end-editor-- Reviewers: @mmoelle1, @mlxd, @yboettcher Archive: 10.5281/zenodo.7735084
Status
Status badge code:
Reviewers and authors:
Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)
Reviewer instructions & questions
@mmoelle1 & @mlxd, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @danielskatz know.
✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨
Checklists
📝 Checklist for @mmoelle1
📝 Checklist for @mlxd
📝 Checklist for @yboettcher