search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
714 stars 38 forks source link

[REVIEW]: LiberTEM: Software platform for scalable multidimensional data processing in transmission electron microscopy #2006

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @uellue (Dieter Weber) Repository: https://github.com/LiberTEM/LiberTEM Version: 0.5.0 Editor: @majensen Reviewers: @alvarolopez, @fedorov Archive: 10.5281/zenodo.3763313

Status

status

Status badge code:

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

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

@alvarolopez & @fedorov, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @majensen know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @alvarolopez

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @fedorov

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @alvarolopez, @mamcdona77 it looks like you're currently assigned to review this paper :tada:.

:star: Important :star:

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:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.5281/zenodo.2450624 is OK
- 10.1145/1327452.1327492 is OK
- 10.1109/nuicone.2012.6493198 is OK
- 10.1109/mcse.2011.37 is OK
- 10.1145/2934664 is OK
- 10.25080/majora-7b98e3ed-013 is OK
- 10.5281/ZENODO.3396791 is OK
- 10.5281/ZENODO.2649351 is OK
- 10.1017/s1431927619000497 is OK
- 10.5281/ZENODO.3572855 is OK

MISSING DOIs

- None

INVALID DOIs

- 10.1142/11389 is INVALID
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

alvarolopez commented 4 years ago

@majensen I am done, the paper is ok to go in its current form.

majensen commented 4 years ago

@alvarolopez - very much appreciated! @mamcdona77 - how are things progressing?

mamcdona77 commented 4 years ago

Will update early next week

Michael Michael A McDonald, MD, PhD

On Jan 22, 2020, at 9:52 PM, Mark Jensen notifications@github.com wrote:

@alvarolopez - very much appreciated! @mamcdona77 - how are things progressing?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

uellue commented 4 years ago

@whedon generate pdf

uellue commented 4 years ago

Would it be OK to include a small wording improvement in the first sentence?

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

majensen commented 4 years ago

@uellue of course!

majensen commented 4 years ago

@mamcdona77 - How is it coming along? Can I help in any way? Thanks

majensen commented 4 years ago

@whedon add @fedorov as reviewer

whedon commented 4 years ago

OK, @fedorov is now a reviewer

majensen commented 4 years ago

Thanks @fedorov for agreeing to review this work.

fedorov commented 4 years ago

@uellue very nice work! I submitted just one issue, and also have few clarification questions below, before I check off the remaining items in the checklist (am not sure if I should submit issues about those):

uellue commented 4 years ago

Hi @fedorov, thank you for the feedback! Good points, we'll work on it.

majensen commented 4 years ago

@whedon remove @mamcdona77 as reviewer

whedon commented 4 years ago

OK, @mamcdona77 is no longer a reviewer

arfon commented 4 years ago

Dear authors and reviewers

We wanted to notify you that in light of the current COVID-19 pandemic, JOSS has decided to suspend submission of new manuscripts and to handle existing manuscripts (such as this one) on a "best efforts basis". We understand that you may need to attend to more pressing issues than completing a review or updating a repository in response to a review. If this is the case, a quick note indicating that you need to put a "pause" on your involvement with a review would be appreciated but is not required.

Thanks in advance for your understanding.

Arfon Smith, Editor in Chief, on behalf of the JOSS editorial team.

uellue commented 4 years ago

Hi @fedorov @majensen FYI, I have been working on a deeper comparison between Dask and LiberTEM both in programming model and benchmarking. That's still WIP. Two things can already be said: First, Dask has improved impressively since I last looked, and second, the comparison has already helped us to identify opportunities for improvement: https://github.com/LiberTEM/LiberTEM/issues/554#issuecomment-601668112

In that sense, many thanks! More details to follow when the comparison is done. :-)

fedorov commented 4 years ago

@uellue thank you for your note. It is very encouraging to see that review made a difference for you, and will ultimately benefit the user of your library!

majensen commented 4 years ago

@uellue - how's it going?

uellue commented 4 years ago

@majensen Work is progressing, I hope to have an update in a week or two!

majensen commented 4 years ago

@uellue - hope you are staying healthy! How are things coming along?

uellue commented 4 years ago

@majensen Progress! It just turned into a bigger update, closing some long-standing TODO items in the process: https://github.com/LiberTEM/LiberTEM/pull/706

majensen commented 4 years ago

@uellue @fedorov (and @alvarolopez ) - checking in. I see Andrey has only the three Functionality items to check off before completion. I know Dieter has been working through a number of improvements. My thought is, if the functionality as it stands (in medias res) is acceptable to Andrey, maybe we can go forward with completion of the review and towards a recommendation for publication. Dieter, you would be free and encouraged to continue development, but with the publication in your pocket. If the editorial powers have an issue with this plan, they will manifest themselves. What do you guys think? Mark

fedorov commented 4 years ago

@majensen I am happy to recommend for publication. The items I have unchecked are:

I made comments about functionality and performance statements made in the accompanying manuscript, and I know @uellue is working on that. I think those issues could be considered blocking if this was a newly introduced package, but this is clearly not the case - the software has a strong and active team of users and developers. It is absolutely appropriate to proceed with the publication.

uellue commented 4 years ago

@majensen @fedorov the installation instructions have been fixed, as far as I am aware. We now have some information on reference datasets, too. Edit: https://libertem.github.io/LiberTEM/sample_datasets.html

The performance bottleneck that we found when addressing the performance tests is mostly fixed. In detail, sparse matrix products with memory-mapped partitions led to large intermediate result buffers that didn't fit the L3 cache. That severely hurt performance of the scipy.sparse library that we use. The same underlying cause applies to a number of other operations. The fix in the back-end is merged LiberTEM/LiberTEM#706, it just has to be used properly in the front-end part of the code, and we have to benchmark it properly. Solving it involved taking on a long-standing item for restructuring the I/O back-end, that's why it took so long.

About related packages, we have it relatively hidden in our acknowledgments (Edit: https://libertem.github.io/LiberTEM/acknowledgments.html#notable-upstream-projects) more as a hat tip towards other projects, but we could also move it to a more visible location like README.rst if you think it would be a valuable resource for users.

In that sense, I could still work on proper benchmarks, but we can also remove performance claims and wrap up the paper before. :-)

fedorov commented 4 years ago

About related packages, we have it relatively hidden in our acknowledgments

What I referred to in my comments were not related packages, but packages similar in functionality. I recall you mention several of those in your paper, but I did not find them in the documentation. I believe it would be helpful for a user who searches for tools implementing certain functionality to understand better how your package is unique relative to other similar packages (if such exist).

In that sense, I could still work on proper benchmarks, but we can also remove performance claims and wrap up the paper before. :-)

👍

majensen commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

majensen commented 4 years ago

Thanks guys. Since the associate editor-in-chief will look askance at unchecked items, let me propose that when @fedorov is satisfied with @uellue 's updates to paper, software, or both, that @fedorov can check the remaining two boxes and ping me here. At that point, I will do a proofing once-over which may result in a minor pull request for wording or reference resolution, then will we call in the EiC.

fedorov commented 4 years ago

@majensen done!

majensen commented 4 years ago

@whedon check references

whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.5281/zenodo.2450624 is OK
- 10.1145/1327452.1327492 is OK
- 10.1109/nuicone.2012.6493198 is OK
- 10.1109/mcse.2011.37 is OK
- 10.1145/2934664 is OK
- 10.25080/majora-7b98e3ed-013 is OK
- 10.5281/ZENODO.3396791 is OK
- 10.5281/ZENODO.2649351 is OK
- 10.1017/s1431927619000497 is OK
- 10.5281/ZENODO.3572855 is OK
- 10.1142/9789811204579_0005 is OK

MISSING DOIs

- None

INVALID DOIs

- None
majensen commented 4 years ago

@uellue - The proof looks good to me, just two items for you to review:

in paper.md line 140:

in paper.bib line 5

If you would rectify these to your liking, what we need next is the DOI to an archive containing the repo. The slight issue between the current Zenodo archive and the paper an sich is the author list. JOSS prefers that the author list on the archive match that on the paper. Would this be difficult to adjust?

uellue commented 4 years ago

Hi @majensen and @fedorov,

Thx for the feedback!

I just opened a pull request in our repository to update the paper. The performance claims in comparison to other projects are removed, since benchmarking would still take some time to implement properly. The LiberTEM reference is updated to 0.5.0. I've asked the co-creators that have since started contributing to LiberTEM if they would like to join as co-authors.

Two co-creators of LiberTEM, Colin Ophus and Jay van Schyndel, had previously declined to be co-authors since the made only small contributions that they felt didn't warrant co-authorship on the paper. Peter Simon and Jaeweon Shin hadn't replied to an invitation to be co-author.

As next steps, I'll wait for response from the new co-authors, update the paper's author list accordingly, merge, and then we can build a new draft here.

fedorov commented 4 years ago

Two co-creators of LiberTEM, Colin Ophus and Jay van Schyndel, had previously declined to be co-authors since the made only small contributions that they felt didn't warrant co-authorship on the paper.

It's yours and their call, but IMHO if they made contribution to the code described in the paper, this most definitely justifies their co-authorship on the paper.

uellue commented 4 years ago

@fedorov yes, I agree that their co-authorship would be justified. We had a discussion about that some time ago and the result was our authorship policy where everybody who contributed to the repository is invited to be co-author.

fedorov commented 4 years ago

It is remarkable how organized your community is. Once you mentioned it, it is obvious that Authorship policy is a must to have, but I am sure I could find quite a few of established projects that don't have one.

uellue commented 4 years ago

Well, we added one when we needed one! :-D The main discussion was actually in the PR for the first draft of this paper: https://github.com/LiberTEM/LiberTEM/pull/460

Main points were that the decision can't be arbitrary, but based on rules, and that we have to remain able to act and fulfill all requirements for authorship of a paper (consent and approval of a draft by all co-authors) even if we have a lot of co-authors in LiberTEM.

uellue commented 4 years ago

@whedon generate PDF

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

uellue commented 4 years ago

Ok, this should be it! :-) Just had a last look at the proof and it looks good.

majensen commented 4 years ago

Great @uellue - can you provide the doi for your archive here- then I will twiddle various knobs and then summon the editors in chief--

uellue commented 4 years ago

Great @uellue - can you provide the doi for your archive here- then I will twiddle various knobs and then summon the editors in chief--

@majensen sounds good! This is the DOI for LiberTEM 0.5.0: https://doi.org/10.5281/zenodo.3763313

uellue commented 4 years ago

@majensen Wait, I noticed something, forgot a co-author!

uellue commented 4 years ago

@whedon generate PDF

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

uellue commented 4 years ago

Ok, looks good now.