-
-
Notifications
You must be signed in to change notification settings - Fork 38
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]: A MATLAB-based Instrument Control (MIC) package for fluorescence imaging #7275
Comments
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:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
|
Software report:
Commit count by author:
|
Paper file info: 📄 Wordcount for ✅ The paper includes a |
License info: ✅ License found: |
@bencardoen, @raacampbell, this is the review thread for the paper. All of our communications will happen here from now on. As a reviewer, the first step is to create a checklist for your review by entering:
as the top of a new comment in this thread. These checklists contain the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. The first comment in this thread also contains links to the JOSS reviewer guidelines. 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, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention #7275 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. Hardware submissions can be tricky to review, as not all reviewers will necessarily be able to review all functionality. I hope with three reviewers we can cover anything. However, if there are aspects of the submission you can't review, please let me know. We aim for reviews to be completed within about 2-4 weeks. Please let me know if any of you require some 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. Please feel free to ping me (@adamltyson) if you have any questions/concerns. |
Review checklist for @raacampbellConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
COMMENTSGeneralThe idea behind this project is excellent. Having an organised collection of classes that control hardware is definitely useful. Structuring it such that classes inherit from abstract base classes to enforce consistency is exactly the correct thing to do. I commend the authors for this. My main reservation a present is that documentation is far too sparse. There really needs to be a good documentation with extensive examples, links to projects that have used the system, etc, etc. Without this, it is very hard envisage the scope of the project. My next reservation is that the organisation and design of project at present confines it to being a useful set of hardware control examples, rather than a well-integrated platform for building hardware-based software projects. Code QualityPM100D classI have not been able to look at all the code and test everything. I looked at a few things and focussed on the ThorLabs PM100D power meter code. I was pleased to see this and did not know it was so easy to talk to the device via VISA. The interface provided by the authors seems rushed, though, and not well thought out. For example:
In summary, to effectively use the PM100D class I had to perform extensive modifications. There are still more changes I would make, but my PR addresses the main problems. Lack of a model/view systemOne basic thing a project of this sort should fulfil is that it should constitute a well-thought through model/view system such that users can easily use the hardware control classes with any arbitrary GUI. Thus, hardware control logic and GUI code should be separate and independent. Hardware control code should have clearly defined and documented standards and behaviours across classes to make it predictable to work with. Hardware control code should have well designed Observable properties to which GUIs can attach listeners in order to facilitate building views in top of the models (hardware control classes). These features are not fulfilled by MIC. The presence of GUI control code in hardware classes particularly problematic. For example, in Hardware settingsOne problem that is encountered when building software systems of this sort is the need to handle hardware settings that might differ between rigs and projects. e.g. stage limits, camera frame rates, etc. This allows software to start up with safe and useful default values or to maintain settings across settings by caching them in a text file. It would have been great, therefore, if MIC included a class for handling hardware settings. e.g. writing them to a file. checking they are within range and valid, maybe linking property changes to values in the file, etc. I have code that largely does these things if the authors would like to adapt it... TestsAll classes seem to have a I realise it can be hard to unit test hardware control code, but I feel that it should be clear what the so-called "unitTest" methods are in the code. Their naming creates an expectation that is not fulfilled. I have suggestions in the Issue, above. OverallBuilding a coherent software toolset for that works together well for hardware control is hard. It requires a lot of planning and really well written classes so users have faith that everything will work as expected. I do not feel that MIC in its current state fulfils these requirements. Some classes do prove to be useful examples for how to do something (e.g. I didn't know I could communicate with the power meter via VISA), but I would take the key information from MIC and roll my own solution instead. I wouldn't try to build a system using MC. The main reasons for this are:
I am sorry if this comes across as negative. I enjoyed reviewing this submission and learned things from it. To my knowledge MATLAB is missing an effective solution of the sort proposed here, but such a solution is actually very hard to do well. MIC represents a first step but is not the whole journey. Issues associated with this reviewIssues highlighted as "BLOCKER" represent items that require fixing before acceptance. The remaining Issues mostly fall into the "nice to have" category, but in some cases not addressing them will substantially reduce the potential impact of the work.
Pull requests |
Review checklist for @bencardoenConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
CommentsOverall, I think this is worthwhile effort, long underway (2018 introducing citation).
WritingWriting is concise and good. However, please do not undersell yourself. For example, you write Tests.
Documentation
Related repository issues:
Executive summaryI thank the authors for sharing this repository with the community, this is a very valuable resource. However, in its current form I would at a minimum like to see the related work section to read discriminative, not be enumerative. Please consider documenting how easy it is to call from Python/Julia. Tests should be automated (publicly), and extended beyond always true abstract cases. Code coverage should be high. The documentation has to be automatically generated (online), and critically should be 1-1 with code. Consider detaching GUI from your controlling code to enable headless mode. (and the listed issues should be resolved). edit: Reflecting on this post review, I think one under-appreciated advantage of MIC is the wealth of supported models, it is not solely an abstract API, but the many existing classes provide a catalog of existing models, so MIC functions not just as control software, but as an open source reference implementation for their usage. Perhaps this can be added to the paper as an explicit strength. |
I am finished with my review. |
Thanks @raacampbell. @sajjad88 you can start looking into addressing these issues now, or you may choose to wait until @bencardoen has had a chance to complete their review. |
Hi @bencardoen, how is the review going? |
@adamltyson apologies, aim to have it completed by the weekend |
@adamltyson How do I link back issues in the source repository to this page? |
@adamltyson I have completed my review |
Hi @bencardoen, thanks for your review. In answer to this question, you already have, by quoting this issue. @sajjad88 I will leave it to you now to work on the issues outlined by the reviewers. If you have any questions, please ping the reviewer or myself. |
Submitting author: @sajjad88 (Sajjad Khan)
Repository: https://github.com/LidkeLab/matlab-instrument-control
Branch with paper.md (empty if default branch):
Version: v0.1.0
Editor: @adamltyson
Reviewers: @bencardoen, @raacampbell
Archive: Pending
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
@bencardoen & @raacampbell, 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:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @adamltyson 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 @raacampbell
📝 Checklist for @bencardoen
The text was updated successfully, but these errors were encountered: