Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign up[REVIEW]: spam: Software for Practical Analysis of Materials #2286
Comments
|
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @Shuang-Plum, @jwbuurlage it looks like you're currently assigned to review this paper Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post. If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews To fix this do the following two things:
For a list of things I can do to help you, just type: For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type: |
|
PDF failed to compile for issue #2286 with the following error: /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/psych.rb:377:in |
|
@Shuang-Plum and @jwbuurlage : Thanks for agreeing to review this paper. Please feel free to contact me via comment or the email in my profile if you need any assistance or if there are questions that need answering. Note, from the pre-review, that the current paper text is in a branch, and that's probably where you should clone from for your review. |
|
@whedon generate pdf from branch spamPaper |
|
|
@jwbuurlage @Shuang-Plum Everything under control? |
|
Hi @edwardando, I will try to find some time this week to start working on the review process. Thanks! |
|
I have concluded my review. I would say overall the software package, documentation, and article meet the JOSS requirements, but I do have some minor comments that I would like to see addressed. |
|
Thanks very much for your time, they are all reasonable and we'll handle them now. |
|
@edwardando -- yes, feel free to make the changes. |
Yes. |
|
@jwbuurlage -- we've handled all your (pertinent and constructive) comments, thanks very much! Just two points of feedback:
@Shuang-Plum Please review this updated version of our website and paper, I'll regenerate the paper now. Please let us know if you need anything from us. |
|
@whedon generate pdf from branch spamPaper |
|
|
@Shuang-Plum My apologies to you and to the authors for not paying closer attention to this paper. When can you get to this review? |
|
@whedon check references |
|
Documentation: Out of band discussion with @Shuang-Plum indicates they will work on the review in the next week. |
|
Perfect, we're looking forward to it! |
|
So sorry for the serious delay!!!!!!! @edwardando I've updated my comments above. Overall, I really like it! I think the software is more powerful than what you've presented. I strongly recommend benchmarking it as it would show how good the software is and would attract more users. I don't think there is any major problem that definitely needs attention. All my comments are suggestions and you can decide whether you would like it or not. |
|
Thanks very much for the review, and positive feedback! |
|
Thanks again for the comments, we've taken most of then into account, here are some notes:
also since we think that we're converging we've merged the spamPaper branch into master, we'll now try to compile a new proof. |
|
@whedon generate pdf from branch master |
|
|
This proof is OK for us! |
|
Noted. I will get back to this this evening. |
|
It looks good! I don't have a strong preference for separating the 'tools provided' and I'm looking forward to seeing the 'twistable parameter' section in the future. For me, the tutorials and examples are sufficient to get me started on spam, but any advanced guidance for users who want to dig further would be really helpful. @edwardando |
|
@whedon check references |
|
|
@edwardando -- See comment above. The invalid DOIs need to be bare DOI's in your .bib file -- remove the prefix as noted. Will you look at the five suggested DOIs to see if they are true matches (and update the .bib file accordingly) or false positives (and note that in a reply comment here)? In your paper:
|
|
@whedon check references |
|
|
@whedon generate pdf from branch master |
|
|
@usethedata Thanks for your careful reading of the paper. We're just a bit worried about those 404s -- unless our website was experiencing outages yesterday, Could we ask you to check them again? |
|
The problem with the links is apparently in how some PDF readers and how the |
|
|
|
On the firefox PDF viewer as well as okular/evince the links work OK. As far as I understand the # references a position in the webpage, so they could safely be removed at the expense of some precision. |
|
Sorry this suggestion causes so much trouble!! If it is a pain and can not be resolved easily, we should let go of the links. I initially suggest it because when I read the text, I would click back to the website for more details. And to be honest, it worked out fine on my end no matter how I open it...... |
|
I think this |
Submitting author: @edwardando (Edward Andò)
Repository: https://gricad-gitlab.univ-grenoble-alpes.fr/ttk/spam/
Version: 0.5.1.4
Editor: @usethedata
Reviewer: @Shuang-Plum, @jwbuurlage
Archive: Pending
Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.
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
@Shuang-Plum & @jwbuurlage, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @usethedata know.
Review checklist for @Shuang-Plum
Conflict of interest
Code of Conduct
General checks
Functionality
It is a smooth experience. As the authors stated, it might require a little more commitment for it to work on Windows. The anaconda tip is really helpful @jwbuurlage. Maybe there could be a brief statement (require Ubuntu as WSL) either in the paper or in gitlab for Windows users. This is a very minor point. It is up to the authors' preference.
Most of the functions work smoothly. I would appreciate it if the authors could list the performance data for some of the more time-consuming functions. Like for x MB 3D time series of x time points, running x function on a xxxxx platform usually takes xx sec/min. I see something similar in the comments of the tutorials. I want to point this out as:
- This would be a great guide and provide proper expectations for new users.
- This could also be a selling point for the software as it can be benchmarked against Numpy or Scipy.
I don't think so. But as mentioned above, it would be great if the authors could benchmark a few corresponding functions against Numpy or Scipy.
Documentation
The authors did a great job stating the general function of this software. As the software has a very general application with 2D and 3D images (sets), I would appreciate a little more details about the input data format requirement upfront (like only a single 3D TIFF per state file is supported for 3D images). It would be essential information the users want to see clearly at the very beginning that whether this tool is applicable to their data. And probably ways to convert image sets (especially for 3D) to acceptable format.
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).
The examples in the tutorials and the gallery are really helpful and they all work.
- As the author mentioned, there are a lot of defaults for the functions (which is always the case). I would appreciate it if the authors could point out key parameters that can be twisted for different purposes along with the examples. Or even better organizing parameter twisting as a separate part of the tutorial.
- A very minor point, the axes of a few plots (for example) in the tutorials and the gallery are really small. To my understanding, the axes don't matter for the illustration purpose of those plots. Would it be better to get rid of the axes labels then?
The authors have done a great job documenting the functionalities.
- I would appreciate it if the 'tools provided' part can be separated out as itself in the main list (right now it is under intro). It would be easier for users to find and search.
- Another point relates to the paper. It would be great if links to 'tools provided' (or even 'Gallery of examples' could be provided there for each function toolkits. This would give the users an easier way to get to know the functions more straightforwardly and in more detail if they want to.
Software paper
As mentioned above, it would be great to mention the input format requirement upfront.
It would be easier to see and compare with the information packed into a table.
- As mentioned above, the toolkit section would be better if links to tutorials or the gallery could be provided.
- The 'Use in existing work' is great with the reference of which part is related. It would be better if the Figure number could also be provided to guide the readers.
Review checklist for @jwbuurlage
Conflict of interest
Code of Conduct
General checks
Edward Andò (@edwardando) appears to be the main contributor in terms of number of commits. All authors that have made code contributions to the software appear to be in the list of paper authors.
Functionality
My Linux distributions (Fedora (rawhide)) was not in the list of supported operating systems, however:
worked fine, and more or less coincided with the installation instructions given in the documentation. I would say the installation procedure is painless, and well documented.
The authors divide the functionality of the software in a number of toolkits. The list of toolkits (or at least how they are grouped together) differs between the software documentation and the article. I prefer the list of toolkits of the software documentation over the list of toolkits given in the article, as the range of functionality is immediately clear. The list in the article is more exhaustive.
(I am not aware of any performance claims or benchmarks comparing the software with competing packages.)
Documentation
Yes, although the first paragraph of that page may benefit from some additional sentences explaining some of the benefits and capabilities of the software.
If you happen to run an OS explicitly mentioned in the installation page then the installation instructions are clear. They do require installation of packages that is handled outside of the Python package manager, which I think is acceptable given the dependencies (in particular MPI). Some thoughts:
The documentation would benefit from a paragraph 'other Linux distributions' where a list of software dependencies is given. This allows users to easily locate the relevant packages from their distribution, instead of decyphering and translating the list of Ubuntu packages given in the example.
The software could be given as a meta-package, with individual toolkits also published as their own packages. This would reduce the number of dependencies if only some of the toolkits need to be used. (I do not know in detail what the inter-dependencies are of the individual toolkits that are part of spam).
Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
Yes, there are elaborate tutorials that are well written. I encountered a broken link while following one of the tutorials (in the sentence "calculate the strains coming from the measured displacement field of this example"). One suggestion would be to include a download link to say a ZIP file of the datasets that are used in the tutorial at the beginning, so that you can download them once and then follow along.
The gallery of examples is excellent. I tried some on my system, and they all worked without fail.
Yes, the API methods is clearly documented. The API documentation is automatically generated and available on the documentation page.
Yes, with acceptable coverage.
Contributions, yes. For issues or seeking support, I could only find a link to a matrix.org chat room. I would suggest adding a section for reporting issues or filing requests.
Software paper
As someone with experience in the field of tomography, it is clear to me what the purpose of the software is, and that it fills an important void. However, I think the summary could benefit from giving some simple examples, in simple words, of 3D image measurement tasks that the software can perform.
Yes, but the section is short, and would benefit from one or two sentences about each software package and what the functionality overlap is between them and
spam.It is acceptable, but I do have a number of comments.
My main critique is that the language is too colloquial in a number of places.
There are some inconsistencies with font usage: some software packages are written using a monospace font, and some are not. I would indeed write NumPy over
numpy, and some official spellings of software packages are all lowercase which complicates matters. However, why aresphinxandunittestmonospace, but tifffile and meshio are not? I would double check all official spellings of packages, for example it is Git not git.Some of the links are not permalinks, and this is even sometimes explicitely marked as such ('currently available at').
References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?