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

[REVIEW]: Armadillo: a template-based C++ library for linear algebra #26

Closed
17 tasks done
whedon opened this issue Jun 9, 2016 · 15 comments
Closed
17 tasks done
Labels
accepted published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. review

Comments

@whedon
Copy link

whedon commented Jun 9, 2016

Submitting author: @conradsnicta (Conrad Sanderson)
Repository: https://github.com/conradsnicta/armadillo/
Version: v7.200.1b
Editor: @arfon
Reviewer: @wrathematics
Archive: 10.5281/zenodo.55251

Status

status

Status badge code:

HTML: <a href="http://joss.theoj.org/papers/4d6506e46a96659b74f48b51ef92fa93"><img src="http://joss.theoj.org/papers/4d6506e46a96659b74f48b51ef92fa93/status.svg"></a>
Markdown: [![status](http://joss.theoj.org/papers/4d6506e46a96659b74f48b51ef92fa93/status.svg)](http://joss.theoj.org/papers/4d6506e46a96659b74f48b51ef92fa93)

Reviewer questions

Conflict of interest

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (such as being a major contributor to the software).

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release (v7.200.1b)?
  • Archive: Does the software archive resolve?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: Have any performance claims of the software been confirmed?

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the functionality of the software documented to a satisfactory level (e.g. API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

Paper PDF: 10.21105.joss.00026.pdf

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g. papers, datasets, software)?
@whedon whedon added the review label Jun 9, 2016
@arfon
Copy link
Member

arfon commented Jun 9, 2016

/ cc @openjournals/joss-reviewers - would anyone be willing to review this submission?

If you would like to review this submission then please comment on this thread so that others know you're doing a review (so as not to duplicate effort). Something as simple as :hand: I am reviewing this will suffice.

Reviewer instructions

  • Please work through the checklist at the start of this issue.
  • If you need any further guidance/clarification take a look at the reviewer guidelines here http://joss.theoj.org/about#reviewer_guidelines
  • Please make a publication recommendation at the end of your review

Any questions, please ask for help by commenting on this issue! 🚀

@wrathematics
Copy link
Member

I am reviewing this.

@wrathematics
Copy link
Member

Done reviewing. I have only a few minor comments.

The DOI for the software repository (incorrectly) lists the license as MPL 1.1, rather than MPL 2.0. This may seem minor, but it's worth nothing that MPL 1.1 is not an OSI approved license, though MPL 2.0 is. I actually had a mild moment of panic when I saw that, because my understanding is MPL 1.1 isn't GPL compatible, which would affect a large number of current users. But both the LICENSE.txt and README.txt files confirm it is being distributed under MPL 2.0.

The only other possible quibble is the performance checkbox (which I did check). No specific performance claims are made, but several vague claims are made to the general effect that it achieves high performance. I have used Armadillo in the past and that is my general impression, although I have not tested this extensively and no benchmarks are provided. I personally would like to see at least some simple benchmarks, for example comparing the overhead of using Armadillo to compute an SVD over calling LAPACK directly. Though that may be beyond the scope of this review, and probably shouldn't be a strict requirement.

I recommend publication, ideally after fixing the license statement in the DOI.

@arfon
Copy link
Member

arfon commented Jun 9, 2016

Thanks for the rapid turnaround @wrathematics!

I recommend publication, ideally after fixing the license statement in the DOI.

@conradsnicta - can you reply on this thread when you've had a chance to address this?

@conradsnicta
Copy link

@arfon - I've fixed the license metadata on the Zenodo archive. It now states "License: Other (Open)". Zenodo currently doesn't have an option for MPL v2.0, so this is the closest choice.

Also, I've incorporated a few minor fixes to the references (paper.bib). Could you pull them in and regenerate the paper PDF?

@arfon
Copy link
Member

arfon commented Jun 10, 2016

Updated paper: 10.21105.joss.00026.pdf

@arfon
Copy link
Member

arfon commented Jun 10, 2016

@arfon - I've fixed the license metadata on the Zenodo archive. It now states "License: Other (Open)". Zenodo currently doesn't have an option for MPL v2.0, so this is the closest choice.

@conradsnicta - sounds good.

@arfon
Copy link
Member

arfon commented Jun 10, 2016

@conradsnicta - I think this is good to accept now. Are there any other changes you're planning on making?

/ cc @wrathematics

@conradsnicta
Copy link

@arfon - no changes from my side.

However, there seems to be an inconsistency in the referencing style in the generated paper (page 2). Specifically, the ordering of the names doesn't appear right: the first author in each reference is listed as Lastname, Firstname, and then following authors are listed as Firstname, Lastname. Is this deliberate?

@arfon
Copy link
Member

arfon commented Jun 10, 2016

However, there seems to be an inconsistency in the referencing style in the generated paper (page 2). Specifically, the ordering of the names doesn't appear right: the first author in each reference is listed as Lastname, Firstname, and then following authors are listed as Firstname, Lastname. Is this deliberate?

Yeah, I'd spotted that too. I'm just using the default bibliographic style from LaTeX/Pandoc.

@conradsnicta
Copy link

@arfon - Maybe try this in the preamble of paper.tex:
\usepackage{natbib}
\setcitestyle{authoryear,open={(},close={)}}
then use \citep{refname} inside the body for citations.
Finally, just before \end{document}:
\bibliographystyle{plainnat}
\bibliography{paper}

@arfon
Copy link
Member

arfon commented Jun 10, 2016

Thanks @conradsnicta, I tried adding most of this to the LaTeX template but it didn't seem to change the PDF output.

then use \citep{refname} inside the body for citations.

I don't think we can do this as we go from the Markdown to PDF (via LaTeX) in a single step here with the citations being handled by pandoc.

I'm afraid we've reached the limit of my pandoc-fu :-. I'd propose that we accept this as-is and file an issue on https://github.com/openjournals/whedon about this. Does that sound OK?

@conradsnicta
Copy link

@arfon - sounds good. Please go ahead.

@conradsnicta
Copy link

@arfon - issue filed: openjournals/whedon#2

@arfon arfon added the accepted label Jun 10, 2016
@arfon
Copy link
Member

arfon commented Jun 10, 2016

@wrathematics many thanks again for the rapid review here!

@conradsnicta - the DOI for this paper is http://dx.doi.org/10.21105/joss.00026 🎉 🚀 💥. The URL doesn't resolve just yet as the Crossref submission queue is a little backed up.

@arfon arfon closed this as completed Jun 10, 2016
@arfon arfon changed the title Submission: Armadillo: a template-based C++ library for linear algebra [REVIEW]: Armadillo: a template-based C++ library for linear algebra Aug 19, 2018
@whedon whedon added published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. labels Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. review
Projects
None yet
Development

No branches or pull requests

4 participants