search

openjournals / joss-reviews

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

[REVIEW]: ImSwitch: Generalizing microscope control in Python #3394

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @kasasxav (Xavier Casas Moreno) Repository: https://github.com/kasasxav/ImSwitch Version: v1.2.0 Editor: @Kevin-Mattheus-Moerman Reviewers: @uellue, @beniroquai, @untzag Archive: 10.5281/zenodo.5196462

:warning: JOSS reduced service mode :warning:

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

Status badge code:

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

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

@uellue & @beniroquai & @untzag, 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 @Kevin-Mattheus-Moerman 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 ✨

Review checklist for @uellue

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @beniroquai

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @untzag

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

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

:warning: JOSS reduced service mode :warning:

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.

: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 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1088/1361-6463/ab4c13 is OK
- 10.1038/s41467-018-05799-w is OK
- doi:10.5281/zenodo.3555620 is OK
- 10.1002/0471142727.mb1420s92 is OK
- 10.1038/s41592-021-01087-6 is OK
- 10.1101/2021.01.18.427171 is OK
- 10.1101/2021.01.18.427178 is OK
- 10.1063/1.4972392 is OK
- 10.5281/zenodo.4433237 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.34 s (582.2 files/s, 62493.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                         182           3566           4541          11557
JSON                             2              0              0            463
reStructuredText                 9            271             89            451
TeX                              1              2              0            131
Markdown                         2             27              0             73
YAML                             1              6              2             39
DOS Batch                        1              8             18              8
-------------------------------------------------------------------------------
SUM:                           198           3880           4650          12722
-------------------------------------------------------------------------------

Statistical information for the repository '5e10c8ce00e37e8d3598860d' was
gathered on 2021/06/22.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Andreas   Bodén                  8           835            304            1.52
Testalab                         2             2              2            0.01
jonatanalvelid                  43          7806           3333           14.91
stafak                         217         29237          18071           63.31
xaviercm94                     113         10896           4244           20.26

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Andreas   Bodén              68            8.1         12.7               16.18
Jonatan Alvelid            1750          100.0          3.4               13.54
jonatanalvelid              410            5.3          0.9               12.68
stafak                    14694           50.3          2.3               14.08
xaviercm94                 2743           25.2          7.5               60.85
whedon commented 3 years ago

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

Kevin-Mattheus-Moerman commented 3 years ago

@MakerTobey, @uellue, @beniroquai, @untzag, this is where the review takes place. Have a look at the instructions above. Let me know if you have questions.

Thanks for your help reviewing this work!

untzag commented 3 years ago

Andreas Bodén and Ilaria Testa are listed as fourth and fifth authors, but I don't see them listed as contributors [1] to the software repository. @kasasxav could you clarify how these individuals contributed to ImSwitch? JOSS authorship guidelines might be relevant here [2].

[1] https://github.com/kasasxav/ImSwitch/graphs/contributors [2] https://joss.readthedocs.io/en/latest/submitting.html#authorship

untzag commented 3 years ago

@kasasxav I feel strongly that ImSwitch should provide setup.py or pyproject.toml so that the software may be installed as a "proper" Python package. To me this issue block publication in JOSS.

See https://github.com/kasasxav/ImSwitch/issues/40, we can discuss there if you need some pointers on how to proceed with packaging.

untzag commented 3 years ago

@kasasxav JOSS requires documentation for contributors / community members. Please provide that prior to publication.

See https://github.com/kasasxav/ImSwitch/issues/44

untzag commented 3 years ago

Due to the nature of review I've just opened a bunch of issues about problems I see with ImSwitch, which can seem quite negative...

I just want to say for the record that I like this project and I think it's definitely worthy of publication after some polish! I like the focus on modularity. The polish of the GUI is impressive. I could imagine some of the scientists that I work with using this software down the road. Good job authors!

xcasas commented 3 years ago

Hi @untzag, thank you very much for your feedback. Thank you also for the nice words about the project.

We are working on addressing the issues you mentioned. As for kasasxav/ImSwitch#44 and kasasxav/ImSwitch#45, we have now included the required documentation.

As for the authors contributions, I write it below:

xcasas commented 3 years ago

I understand your concern @untzag, and I will try to clarify it for you. I believe that in our project, there have been two phases: First, we designed the architecture of the software, applied the MVP into our project, and decided how we wanted to create the Python classes and connections between the different modules of ImSwitch. In this initial step, it was mainly Andreas Bodén and me who discussed it. Some of the ideas that we brought together and have been important in the development of the project are:

Andreas also developed different classes like the first versions of the ScanDesigners (https://github.com/kasasxav/ImSwitch/commit/13af1bd62dafe11a25e6ea9c72393c5eea979bdf) and NidaqHelper (https://github.com/kasasxav/ImSwitch/commit/8f1d2ebb12ece49ea5eb3be1d8879e32f60c2781). The Image Reconstruction module was also based on his Python code, but since we re-arranged it using an MVP and Staffan introduced it the code, it is not listed in GitHub. We took it from his private repository.

The second phase is when Staffan and Jonatan joined the team. We have focused on providing functionality and improving the design, so it has been quite productive in developing code and making a stable release. But it has been possible because we had solid ground from the beginning and a clear idea about how the code should be.

Ilaria Testa has provided direction and guidance to the project. In her career, she has been very involved in the super-resolution community and worked with a wide variety of microscopes and control software. She has given feedback regarding what users expect of software and what is needed when building a microscope. For example, we have brainstormed together the idea of annotating the images with parameters from the scan, such as laser powers, region of interest and exposure time. We have also periodically discussed what direction in the development of the code we wanted to take, taking into account future projects and what is needed in the community. For example, we had the idea of writing scripts to control ImSwitch inspired by high-throughput microscopy and long-term recordings (>12 hours).

whedon commented 3 years ago

:wave: @uellue, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @MakerTobey, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @beniroquai, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @untzag, please update us on how your review is going (this is an automated reminder).

uellue commented 3 years ago

👋 @uellue, please update us on how your review is going (this is an automated reminder).

Probably I can get to it next week. Thank you for your understanding!

untzag commented 3 years ago

Thanks for your response regarding authorship, @kasasxav, I'm satisfied that Andreas Bodén and Ilaria Testa are appropriate authors.

untzag commented 3 years ago

@kasasxav I feel strongly that ImSwitch should provide setup.py or pyproject.toml so that the software may be installed as a "proper" Python package. To me this issue block publication in JOSS.

See kasasxav/ImSwitch#40, we can discuss there if you need some pointers on how to proceed with packaging.

The authors have packaged ImSwitch and pushed it to PyPI :clap:

They are also publishing standalone bundles for WIndows. I don't have easy access to a Windows machine at the moment---perhaps another reviewer could try installing using that route.

untzag commented 3 years ago

@kasasxav JOSS requires documentation for contributors / community members. Please provide that prior to publication.

See kasasxav/ImSwitch#44

The authors have satisfied this requirement to my satisfaction, although other reviewers may of-course disagree.

untzag commented 3 years ago

All of my check-boxes have been checked.

I haven't been able to test ImSwitch with real hardware, but I've looked at the source code and I'm satisfied that everything seems reasonable. From what I can see, ImSwitch's contribution isn't raw hardware support but rather orchestration and GUI with a hardware abstraction model. It seems flexible enough to take on a variety of camera-based workloads. I think in cases like these where specialty hardware is involved we can take the authors at their word that the software does work on real production experimental systems.

I personally recommend ImSwitch for publication in JOSS. Look forward to reading what other reviewers think :smile:

beniroquai commented 3 years ago

All my boxes are checked too. Nevertheless, I would really like to implement some of our hardware adapters in this framework since I not only find it visually appealing but like its performance and integration into e.g. Napari. The paper together with the nicely documented instructions give precise details for users. I would like to give it 1-2 more days until I have an opinion about its ease of letting other integrating adapters :-)

Other than that: I like the project a lot and recommend ImSwitch for a publication in JOSS too!

uellue commented 3 years ago

First of all, congratulations! A nice paper and software. A few items before I can check all the boxes:

Functionality

Unfortunately I ran into issues with the GUI on my system: https://github.com/kasasxav/ImSwitch/issues/59 If possible, I'd abstain from feedback for this item and follow the other reviewers. :-)

State of the field

Documentation: Automated tests and community guidelines

Here one could use a bit more documentation, possibly better coverage and perhaps automated coverage reports.

Functionality documentation

After reading the documentation with example configurations and the Hardware Control Configuration section I feel like I would need a lot more detailed reference documentation until I could customize ImSwitch for my own use. Since customization is pitched as the main purpose of ImSwitch, this part of the documentation should be extended. In particular, all items and options that I can use in a JSON parameter file should be documented clearly and concisely. Or did I miss this somehow in the documentation?

Furthermore, more instructions on how to implement extensions such as support for new hardware or analysis methods should be included, or it should be easier to find.

General feedback

This is not relevant for the JOSS review. Reading the functionality documentation, I was wondering if the JSON files to customize ImSwitch for different techniques could lead to the inner platform effect over time. When including more hardware and supporting more techniques, the syntax will likely become more and more complex. Maybe it could be better to use a Python script instead of a JSON file to specify a microscope set-up? That way one could use an autogenerated API reference from docstrings as well, instead of having a "language break" between JSON and Python. That would make addressing my previous comment easier as well.

uellue commented 3 years ago

I don't have easy access to a Windows machine at the moment---perhaps another reviewer could try installing using that route.

@untzag the bundle is confirmed to work for me, minus https://github.com/kasasxav/ImSwitch/issues/59 which I think is unrelated to the installation method.

uellue commented 3 years ago

@kasasxav reading my review comment again, it may sound like a long list of complaints. :-D Just to make it clear, ImSwitch really looks like a nice project, and thank you for releasing it as Open Source! The raised points are to be understood as suggestions to make it even better and to address points that will likely be important to reach a wider audience so that ImSwitch can achieve its full potential. 👍

xcasas commented 3 years ago

Hi @uelle, thank you for your feedback!

Functionality

The issue kasasxav/ImSwitch#59 is now addressed, thank you for bringing it up.

State of the field

We totally agree with your points, ImSwitch aims at providing a solution for microscopy in general but in practice we support specifically light-based microscopy. I have added a sentence in the Summary of the paper now:

"Currently, ImSwitch provides support for light microscopy techniques but could be extended to other microscopy modalities requiring multiple hardware synchronization."

And we have also cited tango controls and EPICS:

“TANGO Controls is a software solution for distributed control systems that provides a communication protocol and an object-oriented architecture for controlling devices of a range of applications. Similar to TANGO Controls, EPICS focuses on supporting applications that operate complex devices such as particle accelerators and telescopes. While their design and architecture are conceptually similar to ImSwitch, our focus resides on microscopy applications and, in particular, image-based control systems and related hardware when performing experiments and handling data (image acquisition, reconstruction, and analysis). We also provide general tools for microscope users and builders of custom-made microscopes.”

Documentation: automated tests and community guidelines

We have added to the documentation how to run tests and where they should be added kasasxav/ImSwitch#60, the config file and the different fields kasasxav/ImSwitch#61, and how to extend ImSwitch kasasxav/ImSwitch#63. I think it looks much more complete now and hopefully clear for both developers that want to extend ImSwitch and users. Thank you again for the feedback.

ImSwitch is a software that aims at facilitating hardware control in microscopy applications. For this reason, in this project we have focused mainly in the imcontrol module, and covered it with automatic tests to make sure that the core functionality works efficiently. The other modules imreconstruct and imscripting have not been covered for the following reasons:

General feedback

Thank you for your suggestion. We use dataclasses that directly fetch the configuration parameters from the JSON files. We have autogenerated a documentation for the JSON files based on docstrings.

We will keep in mind your other suggestions for the future.

xcasas commented 3 years ago

You can check the latest documentation at https://imswitch.readthedocs.io/en/latest/

uellue commented 3 years ago

@kasasxav Thank you for the documentation updates, a great improvement! I've checked the corresponding checkboxes.

When I wanted to test the fix for the "window problem" https://github.com/kasasxav/ImSwitch/issues/59 I ran into a new issue: https://github.com/kasasxav/ImSwitch/issues/73

Do you test these examples in your CI pipeline, actually? From my personal experience it is well worth the effort to automate checks for all examples to make sure the first impression for new users is good. :-)

stafak commented 3 years ago

Thank you @uellue! :) Sorry about the regression in kasasxav/ImSwitch#73, I think I've fixed it now.

We do test a setup similar to example_monalisa.json in our CI pipeline, but you're right that it would be good to run the tests for all examples. (However, in this case I believe we didn't catch the error for a different reason. From the error message, it looks like it happened because NI-DAQ drivers are installed on your computer but the devices are not connected, but the CI pipeline environment doesn't have the drivers installed at all.)

uellue commented 3 years ago

@stafak @kasasxav Thank you for fixing https://github.com/kasasxav/ImSwitch/issues/73 so quickly! Everything seems to work now. :-)

@Kevin-Mattheus-Moerman that concludes my review and I cordially recommend this paper for publication. 💯

beniroquai commented 3 years ago

@Kevin-Mattheus-Moerman I have also finished my test on including external hardware successfully. Very neat software!

Kevin-Mattheus-Moerman commented 3 years ago

I have spoken to @MakerTobey and he is no longer able to review this. I will remove him as a reviewer and proceed to process this submission for acceptance.

Kevin-Mattheus-Moerman commented 3 years ago

@whedon remove @MakerTobey as reviewer

whedon commented 3 years ago

OK, @MakerTobey is no longer a reviewer

Kevin-Mattheus-Moerman commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

Kevin-Mattheus-Moerman commented 3 years ago

@whedon check references

whedon commented 3 years ago

Checking the BibTeX entries failed with the following error:

Failed to parse BibTeX on value "," (COMMA) [#, "@", #, {:title=>["EPICS architecture"], :author=>["Dalesio, L R and Kozubal, A J and Kraimer, M R"], :doi=>[""], :url=>["https://www.osti.gov/biblio/6110347"], :journal=>[""]}, ",", "number", "="]

Kevin-Mattheus-Moerman commented 3 years ago

@openjournals/dev can you see what is going on here? Paper compiles fine but the reference checking fails.

Kevin-Mattheus-Moerman commented 3 years ago

@kasasxav I proofread the paper and it looks good. The references should be checked (see my attempt above :point_up_2:) but other than that I think it looks in good shape. At this point can you work on the steps below:

Thanks

xcasas commented 3 years ago

@whedon check references

whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1088/1361-6463/ab4c13 is OK
- 10.1038/s41467-018-05799-w is OK
- doi:10.5281/zenodo.3555620 is OK
- 10.1002/0471142727.mb1420s92 is OK
- 10.1038/s41592-021-01087-6 is OK
- 10.1101/2021.01.18.427171 is OK
- 10.1101/2021.01.18.427178 is OK
- 10.1063/1.4972392 is OK
- 10.5281/zenodo.4433237 is OK

MISSING DOIs

- None

INVALID DOIs

- None
xcasas commented 3 years ago

Thank you @Kevin-Mattheus-Moerman for the instructions and for your time. We would also like to thank the reviewers @untzag, @beniroquai, @uellue for your time and suggestions, it's truly exciting how the software has improved through this process!

The software version is now v1.2.0 and the Zenodo DOI 10.5281/zenodo.5196462 (https://zenodo.org/record/5196462#.YRaSXC0RoUs).

I also checked that the meta-data matched the one of the paper.

Kevin-Mattheus-Moerman commented 3 years ago

@whedon set version as v1.2.0

whedon commented 3 years ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@whedon commands
Kevin-Mattheus-Moerman commented 3 years ago

@whedon set v1.2.0 as version

whedon commented 3 years ago

OK. v1.2.0 is the version.

Kevin-Mattheus-Moerman commented 3 years ago

@whedon set 10.5281/zenodo.5196462 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.5196462 is the archive.

Kevin-Mattheus-Moerman commented 3 years ago

@whedon recommend-accept

whedon commented 3 years ago 
Attempting dry run of processing paper acceptance...