search

openjournals / joss-reviews

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

[REVIEW]: Delta-Rice: A HDF5 Compression Plugin optimized for Digitized Detector Data #6598

Closed editorialbot closed 4 months ago

editorialbot commented 6 months ago

Submitting author: !--author-handle-->@david-mathews-1994<!--end-author-handle-- (David Mathews) Repository: https://github.com/david-mathews-1994/deltarice Branch with paper.md (empty if default branch): Version: v1.0.0 Editor: !--editor-->@emdupre<!--end-editor-- Reviewers: @macrocosme, @DanNixon, @dineshchitlangia Archive: 10.5281/zenodo.11490673

Status

status

Status badge code:

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

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

@macrocosme & @DanNixon & @dineshchitlangia, 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:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @emdupre 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 @DanNixon

📝 Checklist for @dineshchitlangia

📝 Checklist for @macrocosme

editorialbot commented 6 months 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 commands

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

@editorialbot generate pdf
editorialbot commented 6 months ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.nima.2020.163961 is OK
- 10.1109/JRPROC.1952.273898 is OK
- 10.1109/TIT.1966.1053907 is OK
- 10.1109/TCOM.1971.1090789 is OK
- 10.1007/s10751-018-1538-7 is OK
- 10.1016/j.ascom.2015.07.002 is OK
- 10.11578/dc.20180330.1 is OK
- 10.13023/etd.2022.446 is OK
- 10.1051/epjconf/201921904002 is OK
- 10.1088/1748-0221/14/11/P11017 is OK
- 10.1088/1748-0221/17/09/P09020 is OK
- 10.5281/zenodo.7568214 is OK
- 10.1109/99.660313 is OK
- 10.1088/1748-0221/14/11/P11017 is OK

MISSING DOIs

- No DOI given, and none found for title: CuPy: A NumPy-Compatible Library for NVIDIA GPU Ca...

INVALID DOIs

- None
editorialbot commented 6 months ago

Software report:

github.com/AlDanial/cloc v 1.90  T=0.05 s (532.5 files/s, 325904.8 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C                                7            484           1126          12087
Markdown                         7            240              0            868
Python                           3            105             82            540
TeX                              1             18              0            189
YAML                             2              3              6             41
Cython                           1             24             13             34
C/C++ Header                     2             12              2             20
diff                             1              1              7              5
DOS Batch                        1              0              0              3
Bourne Shell                     1              0              0              2
-------------------------------------------------------------------------------
SUM:                            26            887           1236          13789
-------------------------------------------------------------------------------

Commit count by author:

   115  david-mathews-1994
    49  David Mathews
     4  dgma224
editorialbot commented 6 months ago

Paper file info:

📄 Wordcount for paper.md is 1605

✅ The paper includes a Statement of need section

editorialbot commented 6 months ago

License info:

✅ License found: MIT License (Valid open source OSI approved license)

editorialbot commented 6 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

emdupre commented 6 months ago

👋 Hi @macrocosme, @DanNixon, and @dineshchitlangia, and thank you for agreeing to review this submission for Delta-Rice !

The review will take place in this issue, and you can generate your individual reviewer checklists here with @editorialbot generate my checklist

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 https://github.com/openjournals/joss-reviews/issues/6598 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.

As you go over the submission, please check any items on your reviewer checklist that you feel have been satisfied. If you aren't sure how to get started, please see the JOSS reviewer guidelines -- and of course, feel free to ping me (@emdupre) with any questions !

We aim for reviews to be completed within four weeks, or six weeks at latest. Please let me know if you require 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.

If you any questions or concerns arise, please feel free to ask here or via email. And thank you again !

dineshchitlangia commented 6 months ago

Review checklist for @dineshchitlangia

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

DanNixon commented 6 months ago

Review checklist for @DanNixon

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

macrocosme commented 6 months ago

Review checklist for @macrocosme

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

emdupre commented 6 months ago

👋 Hi everyone, and happy Monday !

I just wanted to check-in on how this review process is going for you. If you have any specific questions or blockers at this point, please don't hesitate to let me know !

And thank you again for agreeing to review Delta-Rice 💐

macrocosme commented 6 months ago

There doesn't seem to be community guidelines. It would be good to add a note. Tests are note specifically provided, but the examples can serve as to define some, so OK for now.

dineshchitlangia commented 6 months ago

@emdupre I have finished my review and updated the checklist.

Overall, the body of work looks good to me.

Thank you for the invitation to review.

macrocosme commented 6 months ago

@emdupre I also have finished. My comments are similar to those of @dineshchitlangia. All in all, the body of work is good. Thank you for the invitation.

david-mathews-1994 commented 6 months ago

Hello, thanks for the feedback so far! I went ahead and accepted the push to correct the typo, that should be on the main branch now. Great catch!

I'd be happy to add some guidelines in a CONTRIBUTING.md file. Do you have any particular examples that I should follow?

DanNixon commented 5 months ago

I've now finished my review, overall the work looks really good to me.

My only points were:

emdupre commented 5 months ago

Thank you for confirming your reviews, @dineshchitlangia @macrocosme and @DanNixon ! And thank you for your responsiveness to date, @david-mathews-1994.

To follow-up on this question :

I'd be happy to add some guidelines in a CONTRIBUTING.md file. Do you have any particular examples that I should follow?

We don't have a specific template that we request, and our accepted papers adopt a range of different formats. In general, though, I commonly see projects create a CONTRIBUTING.md (as GitHub recognizes these files). Here is one example CONTRIBUTING.md file from a project recently published in JOSS.

If you have any other questions, please don't hesitate to let me know !

david-mathews-1994 commented 5 months ago

@emdupre I added in a contribution guide to the repo and cleaned up the files that @DanNixon identified. I'll look into an automated test suite for the repository as well, and will probably expand the testing to be a bit more thorough than what is presently defined in the example scripts (which I will probably try to use in the automated testing).

What is the path forward for this submission at this stage? This is my first JOSS submission so I'm not familiar with what the next steps are.

emdupre commented 5 months ago

What is the path forward for this submission at this stage? This is my first JOSS submission so I'm not familiar with what the next steps are.

At this point, we ask that you address all of the comments / issues that the authors have raised and then respond in this thread with any remaining questions or blockers. This will give the reviewers a chance to see if your changes have addressed their concerns !

Specifically, given @dineshchitlangia 's comment :

Recommended to add multiple test cases covering some positive and negative test case scenarios and edge cases if any. Such test cases not only ensure the correctness but it also make it more reliable and hence easy to trust/adopt.

and @DanNixon 's issue : https://github.com/david-mathews-1994/deltarice/issues/3

It seems that the remaining points are on developing a more extensive -- and automatic -- test suite.

Please let me know if this is clear, or if you have any other questions re : process that I can help with !

david-mathews-1994 commented 5 months ago

@emdupre Hello again! I think I have created the automated test suite. I did some testing on a different branch and it seemed to work just fine. Just pushed those updates to main and they're currently being tested. I will say it's a bit slow, but it does seem to be running properly at least which is good. Effectively it's checking the compilation and the proper running of the pythonExample.py script.

emdupre commented 5 months ago

Thank you, @david-mathews-1994 ! It looks like these changes to the codebase are associated with these new testing additions :

👋 @macrocosme & @DanNixon & @dineshchitlangia, please let us know whether these effectively changes address your concerns or if you see any remaining issues !

In particular, I noticed that you have not checked the "Community guidelines" criteria, @dineshchitlangia. If you have particular feedback there, please let us know 🙏

DanNixon commented 5 months ago

:+1: Yep, the changes look good to me, and there are no more outstanding issues as far as I am concerned.

danielskatz commented 4 months ago

👋 @emdupre - it looks like this is ready to start finalizing?

emdupre commented 4 months ago

Thanks for checking in, @danielskatz ! I'm still waiting on final reviewer sign-off, in particular re: @dineshchitlangia 's points on the community guidelines criteria.

@dineshchitlangia and @macrocosme, please let us know if you've had a chance to re-review Delta-Rice and are happy with the state of the submission so we can move this forward ! :rocket:

dineshchitlangia commented 4 months ago

@emdupre No further concerns from my ends. All my suggestions have been accounted for and appropriate changes have been made by @david-mathews-1994 . Thank you for your patience while I took some time to review.

emdupre commented 4 months ago

Thank you for confirming, @dineshchitlangia ! I appreciate your response.

@macrocosme, please let us know if you are also happy with the current status of the software. If I do not hear from you by the end of this week, 7 July we will need to move forward with processing this submission for Delta-Rice.

And thank you, everyone, for your engagement in this review process to date !

macrocosme commented 4 months ago

Apologies for the delay. The state of the software looks good.

emdupre commented 4 months ago

Thank you, @macrocosme ! I really appreciate your confirmation 💐

@david-mathews-1994, now that the reviewers are satisfied with the software, I'll go ahead and perform a few editorial checks to move this submission forward.

emdupre commented 4 months ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

emdupre commented 4 months ago

@editorialbot generate pdf

editorialbot commented 4 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

emdupre commented 4 months ago

@editorialbot check references

editorialbot commented 4 months ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.nima.2020.163961 is OK
- 10.1109/JRPROC.1952.273898 is OK
- 10.1109/TIT.1966.1053907 is OK
- 10.1109/TCOM.1971.1090789 is OK
- 10.1007/s10751-018-1538-7 is OK
- 10.1016/j.ascom.2015.07.002 is OK
- 10.11578/dc.20180330.1 is OK
- 10.13023/etd.2022.446 is OK
- 10.1051/epjconf/201921904002 is OK
- 10.1088/1748-0221/14/11/P11017 is OK
- 10.1088/1748-0221/17/09/P09020 is OK
- 10.5281/zenodo.7568214 is OK
- 10.1109/99.660313 is OK
- 10.1088/1748-0221/14/11/P11017 is OK

MISSING DOIs

- No DOI given, and none found for title: CuPy: A NumPy-Compatible Library for NVIDIA GPU Ca...

INVALID DOIs

- None
dineshchitlangia commented 4 months ago

@emdupre Question - Is it possible to also tie the reviewer's ORCid for tracking?

emdupre commented 4 months ago

@dineshchitlangia Unfortunately this is not yet possible, though it is in the pipeline for future development (see https://github.com/openjournals/joss/issues/1286 ) !

For now, the recommendation would be to add your ORCID to your JOSS reviewer profile (https://reviewers.joss.theoj.org) and make sure that your GitHub and ORCID are linked. In the future, this should help us to collate your completed reviews. Apologies that there is not a more immediate solution, however.

david-mathews-1994 commented 4 months ago

@emdupre Hello! I've linked the repo with Zenodo and have the following DOI for it: https://doi.org/10.5281/zenodo.11490673. It doesn't seem to link properly yet, I'm hoping that's just a bit of a delay since I just put that out there. The version tag is v1.0.0.

emdupre commented 4 months ago

Thank you, @david-mathews-1994 ! I do have a few editorial requests :

On the repository:

On the software paper:

Also, apologies, but I noticed that you're using the AAS collaboration template. This wasn't flagged during the pre-review, so I just want to confirm : is this manuscript a companion to an AAS submission (as described here) ?

david-mathews-1994 commented 4 months ago

@emdupre Apologies for the delay on this, I was out of the office for a bit.

I can fix the tests. I was in the process of trying to make the software installable via pip and was working on a automated wheels deployment but haven't quite gotten that to work correctly. I'll hopefully have that fixed later today.

I've reached out to those collaborators and am waiting on a reply from them currently.

Regarding footnote [1], that is text that is required from my employer for acknowledgement of funding/resources. I've received push-back from them in the past when it wasn't on the first page for other journal submissions, so if possible I'd like to keep it there. If that isn't possible, then I can just tell them that it isn't allowed by the journal and I think that'll be okay so long as it remains in the document.

Regarding the template, this is not an AAS submission. I was simply following the example here [https://joss.readthedocs.io/en/latest/example_paper.html] and didn't modify that part of the document. What would you recommend I change that to?

emdupre commented 4 months ago

Thanks for following up ! Please see inline responses below.

I can fix the tests. I was in the process of trying to make the software installable via pip and was working on a automated wheels deployment but haven't quite gotten that to work correctly. I'll hopefully have that fixed later today.

I've reached out to those collaborators and am waiting on a reply from them currently.

Great, thank you ! Please let me know when you have updates on these points.

Regarding footnote [1], that is text that is required from my employer for acknowledgement of funding/resources. I've received push-back from them in the past when it wasn't on the first page for other journal submissions, so if possible I'd like to keep it there. If that isn't possible, then I can just tell them that it isn't allowed by the journal and I think that'll be okay so long as it remains in the document.

I reached out to other members of the editorial team on this ; they noted that the terms listed in the notice are already compatible with JOSS policies because authors retain copyright and CC-BY (the license under which the paper text is release) is entirely compatible with the terms above. I'll also note that since we generate the final PDF directly from the software repository, there isn't a separate step to remove this notice for the version of record. Given that, do you still want to include it in the manuscript ? If so, I think the acknowledgements is the right place for it. Otherwise, you can also point back to this (publicly available) review comment thread to note that JOSS acknowledges the statement.

Regarding the template, this is not an AAS submission. I was simply following the example here [https://joss.readthedocs.io/en/latest/example_paper.html] and didn't modify that part of the document. What would you recommend I change that to?

You should remove the associated optional fields if this is not an AAS associated submission ; specifically, these lines :

# Optional fields if submitting to a AAS journal too, see this blog post:
# https://blog.joss.theoj.org/2018/12/a-new-collaboration-with-aas-publishing
aas-doi: 10.3847/xxxxx <- update this with the DOI from AAS once you know it.
aas-journal: Astrophysical Journal <- The name of the AAS journal.
david-mathews-1994 commented 4 months ago

So in speaking with others at my work, it doesn't sound like we are required to post the footnote in the published version of the document so long as the publication acknowledges the requirements and I believe your last message satisfies that requirement which is great! I've gone ahead and removed it from the document. I also removed the AAS lines and the newest version of the PDF should reflect these changes.

I also fixed the tests failing issues and all are passing now. Just forgot to remove a line in the setup.py script that was breaking things from a previous push (this is why tests are great though!).

emdupre commented 4 months ago

@editorialbot generate pdf

editorialbot commented 4 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

emdupre commented 4 months ago

@editorialbot check references

editorialbot commented 4 months ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.nima.2020.163961 is OK
- 10.1109/JRPROC.1952.273898 is OK
- 10.1109/TIT.1966.1053907 is OK
- 10.1109/TCOM.1971.1090789 is OK
- 10.1007/s10751-018-1538-7 is OK
- 10.1016/j.ascom.2015.07.002 is OK
- 10.11578/dc.20180330.1 is OK
- 10.13023/etd.2022.446 is OK
- 10.1051/epjconf/201921904002 is OK
- 10.1088/1748-0221/14/11/P11017 is OK
- 10.1088/1748-0221/17/09/P09020 is OK
- 10.5281/zenodo.7568214 is OK
- 10.1109/99.660313 is OK
- 10.1088/1748-0221/14/11/P11017 is OK

MISSING DOIs

- No DOI given, and none found for title: CuPy: A NumPy-Compatible Library for NVIDIA GPU Ca...

INVALID DOIs

- None
david-mathews-1994 commented 4 months ago

I've reached out to my collaborators and found the two missing ORCID numbers so that is taken care of. I believe I've also standardized the references to Gzip at this time.

Regarding the table, how do I actually put in a caption for that table within Markdown or in such a way that the Latex interpreter system understands it? I know how to do this in pure Latex, but am not sure how to add a caption to a non-figure object in a way that this PDF generator understands

emdupre commented 4 months ago

Thank you, @david-mathews-1994 !

The JOSS docs should cover this point, but it looks like that section has a formatting issue ; I'll submit an issue on this, so thanks for flagging !

In general, this can be done as simply as adding the following line before the table :

Table 1: An example of Rice coding when writing $-2, 25$ with $m=8$

You can add a label as well, as discussed in the documentation, s.t. it can be directly referenced in text, replacing "in the table below."

david-mathews-1994 commented 4 months ago

@emdupre I ran into a bunch of issues trying to get the table to work so I ended up just making the table in external software as an image and put it into the document that way. Let me know if that formatting looks better!

emdupre commented 4 months ago

@editorialbot generate pdf

editorialbot commented 4 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

emdupre commented 4 months ago

Thank you, @david-mathews-1994 !

I realized I overlooked formatting request, apologies : Currently, affiliations include just the relevant institute. Could you please list each affiliation in a "Department, Institute/Univ., City, Country" format ? Or as close to that format as possible, recognizing that some institutes do not have departments.