openjournals / joss-reviews

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

[REVIEW]: Quantum Instrumentation Control Kit - Defect Arbitrary Waveform Generator (QICK-DAWG): A Quantum Sensing Control Framework for Quantum Defects #6380

Open editorialbot opened 6 months ago

editorialbot commented 6 months ago

Submitting author: !--author-handle-->@egriendeau<!--end-author-handle-- (Emmeline Riendeau) Repository: https://github.com/sandialabs/qick-dawg Branch with paper.md (empty if default branch): paper Version: 0.1.0 Editor: !--editor-->@phibeck<!--end-editor-- Reviewers: @14shreyasp, @ktahar, @sidihamady Archive: Pending

Status

status

Status badge code:

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

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

@14shreyasp & @ktahar & @sidihamady, 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 @phibeck 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 @sidihamady

📝 Checklist for @ktahar

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
Software report:

github.com/AlDanial/cloc v 1.88  T=2.21 s (193.3 files/s, 174946.4 lines/s)
-----------------------------------------------------------------------------------
Language                         files          blank        comment           code
-----------------------------------------------------------------------------------
XML                                 84             44             32         279370
Verilog-SystemVerilog              112           4687           4890          16865
VHDL                                75           2045           5173          16365
Tcl/Tk                              28           1221           1136          14610
Python                              55           3016           5742          10114
JSON                                 2              0              0           3175
SVG                                  9              8              8           2328
Jupyter Notebook                    19              0          10016           2220
Markdown                            15            305              0            746
Perl                                 2            146            132            576
DOS Batch                            2             39              5            231
MATLAB                               5             64             73            198
make                                 2             36              6            171
TeX                                  4              9              0            120
reStructuredText                     4             67             74            104
YAML                                 4             13             35             89
Assembly                             2             11             19             33
CSS                                  1              3              3             15
Bourne Shell                         2              3              5             12
-----------------------------------------------------------------------------------
SUM:                               427          11717          27349         347342
-----------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 6 months ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1063/5.0076249 is OK
- 10.1145/3529397 is OK
- 10.1109/QCE53715.2022.00123 is OK
- 10.48550/arXiv.2309.17233 is OK
- 10.5281/zenodo.1478113 is OK
- 10.1038/natrevmats.2017.88 is OK
- 10.1088/2633-4356/ace095 is OK
- 10.1063/5.0083774 is OK
- 10.1126/sciadv.abg8562 is OK
- 10.1109/TQE.2021.3116540 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 6 months ago

Wordcount for paper.md is 1463

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:

ktahar commented 6 months ago

I have a few concerns before starting the review. Substantial part of this project may be the firmware for specific hardware (RFSoC eval board) and I don't have it now. Situation is similar to the case of pySLM2 #6205.

@phibeck Can I proceed even in this situation? Maybe we must ask the authors to create some mocks to emulate the hardware on PC, in order to check if the software part works as claimed.

@egriendeau Can you possibly include the "modified qick" part in the repository using git submodule (instead of just putting the qick directory)? I would like to see the difference between your modified qick and the original. Having a separate qick repository (with history of original qick) makes this much easier.

phibeck commented 6 months ago

Thanks @ktahar for this initial assessment, that's a good point. @egriendeau could you clarify to what extent access to the hardware is needed to assess the quality of the package? I also have trouble opening the README linked on the repo for installation instructions, please check whether this link needs to be updated, thanks.

ammounce commented 6 months ago

We believe that the package likely stands well on it's own, with out an actual test on hardware:

The main entry point for reviewer or qick-dawg user is the demo jupyter notebook. If we were to make a server for hardware access, the reviewers would run the demo notebook cell by cell and generate output very similar to what is already shown (whlie reading the mark-down cells). Changing parameters on the demo notebook might be do-able by someone with minimal experience with nitrogen vacancy magnetometry/quantum sensing, but changing parameters in the notebook might prove challenging for those with no experience in the field or similar fields.

Nonetheless, we've extensively documented our package. Additionally, we've provided installation, hardware setup instructions, and extensive documentation within our demo jupyter notebook.

We're happy to consider whatever is the best path here.

egriendeau commented 6 months ago

Additionally, I corrected the link from the main readme to the installation readme

ktahar commented 6 months ago

@ammounce Thank you for explanation. I will start my review by playing with the notebook. As I have minimal experience of diamond NV experiments, I think I can get the feel for it.

sidihamady commented 6 months ago

Review checklist for @sidihamady

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

phibeck commented 6 months ago

Thank you, @egriendeau for fixing the link and @ammounce for clarifying the hardware requirements. Sounds like it should be doable to assess the software via the notebook to a good extent.

Thanks @sidihamady, @14shreyasp and @ktahar for getting the review started. Let me know if you have any questions as you work through your checklists.

ammounce commented 6 months ago

@ktahar @sidihamady, @14shreyasp A little more clarification: you do need the hardware to run the demo notebook too.

My comment was that perhaps the demo notebook (as previously) run, could be sufficient. However, if not, let us know and we'll try to setup a test server. I believe this is possible, but could be challenging due to the fact that we're at at National Laboratory and internet security is tight. Nonetheless, I might have a path to making a full test server.

ktahar commented 6 months ago

Hi @ammounce , Thank you for clarification. I was misunderstanding that demo notebook can be run without hardware. But I will try to start the review anyway.

ktahar commented 6 months ago

Review checklist for @ktahar

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

phibeck commented 6 months ago

@ktahar @sidihamady, @14shreyasp A little more clarification: you do need the hardware to run the demo notebook too.

My comment was that perhaps the demo notebook (as previously) run, could be sufficient. However, if not, let us know and we'll try to setup a test server. I believe this is possible, but could be challenging due to the fact that we're at at National Laboratory and internet security is tight. Nonetheless, I might have a path to making a full test server.

Thank you for clarifying @ammounce. According to our policy, the reviewers must be able to test the software. In order to go forward with the review, we do therefore need you to provide a way for the reviewers to run the software, for example via software emulation as suggested by @ktahar. One alternative is to have reviewers with access to the hardware. Perhaps you can let me know if you can think of someone without COI who does have access. Please let me know in which direction you want to proceed. I will place the submission on hold for now until this situation is resolved.

@ktahar @sidihamady, @14shreyasp, thanks again for agreeing to review. Since we do require that reviewers must be able to test the software, this submission is paused for now. I'll keep you updated whether the review is continued at a later point provided there's a way of testing, or whether new reviewers will be assigned who have access to the hardware.

ktahar commented 6 months ago

@phibeck I understood the JOSS policy and your decision. Thank you.

ammounce commented 6 months ago

@phibeck @ktahar @sidihamady Thanks for the clarification on policy, and apologies for missing that point. I'm away at a conference right now, but in the next few weeks i'll have a definitive answer on the test server and get back to you. It's looking promising though!!