editorialbot commented 1 year ago

Submitting author: !--author-handle-->@czimm79@Kevin-Mattheus-Moerman<!--end-editor-- Reviewers: @roflmaostc, @mkitti, @BioTurboNick Archive: 10.5281/zenodo.10578580

Status

Status badge code:

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

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

@roflmaostc & @mkitti & @BioTurboNick, 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 @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 ✨

Checklists

📝 Checklist for @roflmaostc

📝 Checklist for @mkitti

📝 Checklist for @BioTurboNick

editorialbot commented 1 year 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 1 year ago

Software report:

github.com/AlDanial/cloc v 1.88  T=0.04 s (1055.9 files/s, 165874.8 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Julia                           18            725            175           2748
TOML                             3            310              1           1420
Markdown                        14            139              0            362
TeX                              1             16              0            259
YAML                             4              1              7            121
-------------------------------------------------------------------------------
SUM:                            40           1191            183           4910
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository

editorialbot commented 1 year ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1126/scirobotics.aav4317 is OK
- 10.1126/sciadv.aat4388 is OK
- 10.1137/141000671 is OK
- 10.1038/nmeth.2019 is OK
- 10.1126/scirobotics.aba5726 is OK
- 10.1002/adma.202002047 is OK
- 10.1006/jcis.1996.0217 is OK
- 10.1038/s41598-022-09177-x is OK
- 10.1002/nano.202100353 is OK
- 10.1021/acsomega.3c00886 is OK
- 10.1038/nmeth.2089 is OK
- 10.5281/zenodo.7670439 is OK
- 10.5281/zenodo.6234110 is OK
- 10.1021/acs.langmuir.3c00701 is OK
- 10.5281/zenodo.6234110 is OK
- 10.5281/zenodo.8028030 is OK

MISSING DOIs

- None

INVALID DOIs

- https://doi.org/10.1016/j.ymeth.2016.09.016 is INVALID because of 'https://doi.org/' prefix

editorialbot commented 1 year ago

Wordcount for paper.md is 832

roflmaostc commented 1 year ago

Review checklist for @roflmaostc

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the https://github.com/czimm79/MicroTracker.jl?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@czimm79) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
[x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
[x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
[x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

editorialbot commented 1 year ago

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

roflmaostc commented 1 year ago

Well written and extensive documentation!

However, I would recommend to upload the data elsewhere instead of uploading ~300MB of images to GitHub. Also, running the default tutorial, it was not quite clear where the data is coming from. Is there a default path? My first run was failing because of path issues and couldn't find out how to fix it.

czimm79 commented 1 year ago

Hi @roflmaostc, thanks for volunteering to review our submission!

After looking into potential alternatives for hosting the example images, it seems like the best practice for Julia packages is to use the Artifacts feature of Pkg.jl. I will get to work on this and create a new fork of MicroTracker which uses Artifacts.

Currently, MicroTracker is designed so the user's working directory is the project folder. This allows the microscopy video, segmenting results, and linking results to stay organized and referenceable throughout the pipeline. This project folder is created using the create_project_here function. Subsequent functions like batch_particle_data_to_linked_data reference the folders inside the project folder to load and save data. We will work on adding some path checking error handing and documentation to clarify the functions that need to be run from the project working directory to try and mitigate path issues.

Kevin-Mattheus-Moerman commented 1 year ago

@mkitti, @BioTurboNick thanks again for your help in reviewing this work for JOSS. Can you please get started and/or update me on review progress? Thanks!

Kevin-Mattheus-Moerman commented 1 year ago

@roflmaostc, thanks for your help too. I see you have all boxes above ticked. Are there any remaining issues from your end or are you done and happy to formulate a recommendation?

roflmaostc commented 1 year ago

For me this is ready for publication!

mkitti commented 1 year ago

Review checklist for @mkitti

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the https://github.com/czimm79/MicroTracker.jl?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@czimm79) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
[x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
[x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
[x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

mkitti commented 1 year ago

Installation

Installation goes quite smoothly. The package also has some Python package dependencies including numpy, pandas, and trackpy. It would be nice to more clearly document this in the Setup part of the documentation with a note that this will install these packages in the default ROOT conda environment. It would be even better if it could better documented how to use a bespoke conda environment for this purpose. I would be happy to work with the authors on developing that documentation since I am involved with PyCall.jl and Conda.jl.

edit: There are two strategies I can think of that could improve the situation.

Encourage the user to create a new conda environment and point Conda.jl to it. This can be done by the environment variables $CONDA_JL_HOME and $CONDA_JL_CONDA_EXE.
Create a new Julia depot by setting the environment variable $JULIA_DEPOT_PATH. This will result in a new conda install within this depot with it's own environment. This is less than ideal though because we may duplicate some of the Julia package installation.

While it would be nice if the author's could automate option 1, the main requirement here is to document that this will install Python packages into the root Conda environment by default and reference the Conda.jl README as to how to configure this install. This also should reference how PyCall.jl is configured since it's possible that PyCall.jl is not configured to use the correct python executable.

Reproducibility

Additionally, for the purpose of the paper, could the authors also document the versions of all dependencies used. For Julia dependencies this could be in the form of a (Julia)Manifest.toml. While this Manifest.toml should not necessarily be at the package root, perhaps it could be included in the paper directory. If a Project.toml is included along with this manifest, then the project could be activated and instantiated. To clarify, the purpose of this pair of Project.toml and Manifest.toml is needed to document the project used to generate the examples in the paper. Additonally, the versions of the Python packages should be documented. This could be done by exporting an environment.yml file via conda. Establishing the versions of all dependencies is required for future reproducibility.

mkitti commented 1 year ago

Community Guidelines

It is documented how to contribute to the software. However documentation on these issues are currently absent.

Report issues or problems with the software
Seek support

Generally for Julia projects like this Github issues may be used and/or one of the Julia community channels such as Slack or Discourse. The authors should explicitly note how they prefer to be contacted, how to report issues, and how to seek support.

mkitti commented 1 year ago

Automated testing

I am experiencing a test failure on my Windows machine. I will investigate further. My guess is that this is related to the Python environment issues mentioned above.

julia> versioninfo()
Julia Version 1.9.3
Commit bed2cd540a (2023-08-24 14:43 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: 48 × Intel(R) Xeon(R) Gold 5220R CPU @ 2.20GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-14.0.6 (ORCJIT, cascadelake)
  Threads: 1 on 96 virtual cores

Test failure in python_interactions.jl

```julia Python functions: Error During Test at C:\Users\kittisopikulm\.julia\dev\MicroTracker\test\python_interactions.jl:10 Got exception outside of a @test PyError ($(Expr(:escape, :(ccall(#= C:\Users\kittisopikulm\.julia\packages\PyCall\ilqDX\src\pyfncall.jl:43 =# @pysym(:PyObject_Call), PyPtr, (PyPtr, PyPtr, PyPtr), o, pyargsptr, kw))))) ValueError("Invalid file path or buffer object type: ") File "C:\Users\kittisopikulm\.julia\conda\3\lib\site-packages\pandas\util\_decorators.py", line 311, in wrapper return func(*args, **kwargs) File "C:\Users\kittisopikulm\.julia\conda\3\lib\site-packages\pandas\io\parsers\readers.py", line 586, in read_csv return _read(filepath_or_buffer, kwds) File "C:\Users\kittisopikulm\.julia\conda\3\lib\site-packages\pandas\io\parsers\readers.py", line 482, in _read parser = TextFileReader(filepath_or_buffer, **kwds) File "C:\Users\kittisopikulm\.julia\conda\3\lib\site-packages\pandas\io\parsers\readers.py", line 811, in __init__ self._engine = self._make_engine(self.engine) File "C:\Users\kittisopikulm\.julia\conda\3\lib\site-packages\pandas\io\parsers\readers.py", line 1040, in _make_engine return mapping[engine](self.f, **self.options) # type: ignore[call-arg] File "C:\Users\kittisopikulm\.julia\conda\3\lib\site-packages\pandas\io\parsers\c_parser_wrapper.py", line 51, in __init__ self._open_handles(src, kwds) File "C:\Users\kittisopikulm\.julia\conda\3\lib\site-packages\pandas\io\parsers\base_parser.py", line 222, in _open_handles self.handles = get_handle( File "C:\Users\kittisopikulm\.julia\conda\3\lib\site-packages\pandas\io\common.py", line 609, in get_handle ioargs = _get_filepath_or_buffer( File "C:\Users\kittisopikulm\.julia\conda\3\lib\site-packages\pandas\io\common.py", line 396, in _get_filepath_or_buffer raise ValueError(msg) Stacktrace: [1] pyerr_check @ C:\Users\kittisopikulm\.julia\packages\PyCall\ilqDX\src\exception.jl:75 [inlined] [2] pyerr_check @ C:\Users\kittisopikulm\.julia\packages\PyCall\ilqDX\src\exception.jl:79 [inlined] [3] _handle_error(msg::String) @ PyCall C:\Users\kittisopikulm\.julia\packages\PyCall\ilqDX\src\exception.jl:96 [4] macro expansion @ C:\Users\kittisopikulm\.julia\packages\PyCall\ilqDX\src\exception.jl:110 [inlined] [5] #107 @ C:\Users\kittisopikulm\.julia\packages\PyCall\ilqDX\src\pyfncall.jl:43 [inlined] [6] disable_sigint @ .\c.jl:473 [inlined] [7] __pycall! @ C:\Users\kittisopikulm\.julia\packages\PyCall\ilqDX\src\pyfncall.jl:42 [inlined] [8] _pycall!(ret::PyCall.PyObject, o::PyCall.PyObject, args::Tuple{IOBuffer}, nargs::Int64, kw::Ptr{Nothing}) @ PyCall C:\Users\kittisopikulm\.julia\packages\PyCall\ilqDX\src\pyfncall.jl:29 [9] _pycall!(ret::PyCall.PyObject, o::PyCall.PyObject, args::Tuple{IOBuffer}, kwargs::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}}) @ PyCall C:\Users\kittisopikulm\.julia\packages\PyCall\ilqDX\src\pyfncall.jl:11 [10] (::PyCall.PyObject)(args::IOBuffer; kwargs::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}}) @ PyCall C:\Users\kittisopikulm\.julia\packages\PyCall\ilqDX\src\pyfncall.jl:86 [11] (::PyCall.PyObject)(args::IOBuffer) @ PyCall C:\Users\kittisopikulm\.julia\packages\PyCall\ilqDX\src\pyfncall.jl:86 [12] jldf_to_pydf(jldf::DataFrame) @ MicroTracker C:\Users\kittisopikulm\.julia\dev\MicroTracker\src\developer_utilities.jl:55 [13] macro expansion @ C:\Users\kittisopikulm\.julia\dev\MicroTracker\test\python_interactions.jl:25 [inlined] [14] macro expansion @ C:\Users\kittisopikulm\.julia\juliaup\julia-1.9.3+0.x64.w64.mingw32\share\julia\stdlib\v1.9\Test\src\Test.jl:1498 [inlined] [15] top-level scope @ C:\Users\kittisopikulm\.julia\dev\MicroTracker\test\python_interactions.jl:11 [16] include(fname::String) @ Base.MainInclude .\client.jl:478 [17] top-level scope @ C:\Users\kittisopikulm\.julia\dev\MicroTracker\test\runtests.jl:54 [18] include(fname::String) @ Base.MainInclude .\client.jl:478 [19] top-level scope @ none:6 [20] eval @ .\boot.jl:370 [inlined] [21] exec_options(opts::Base.JLOptions) @ Base .\client.jl:280 [22] _start() @ Base .\client.jl:522 Test Summary: | Pass Error Total Time Python functions | 4 1 5 9.5s ERROR: LoadError: Some tests did not pass: 4 passed, 0 failed, 1 errored, 0 broken. in expression starting at C:\Users\kittisopikulm\.julia\dev\MicroTracker\test\python_interactions.jl:10 in expression starting at C:\Users\kittisopikulm\.julia\dev\MicroTracker\test\runtests.jl:54 ERROR: Package MicroTracker errored during testing ```

Also, I'm noting that the automated Github Actions testing does pass. However, there is some output that should be investigated concerning the plot tests.

qt.qpa.xcb: could not connect to display 
qt.qpa.plugin: Could not load the Qt platform plugin "xcb" in "" even though it was found.
This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.

Available platform plugins are: linuxfb, minimal, offscreen, vnc, xcb.

Aborted (core dumped)
connect: Connection refused
GKS: can't connect to GKS socket application

GKS: Open failed in routine OPEN_WS
GKS: GKS not in proper state. GKS must be either in the state WSOP or WSAC in routine ACTIVATE_WS
qt.qpa.xcb: could not connect to display 
qt.qpa.plugin: Could not load the Qt platform plugin "xcb" in "" even though it was found.
This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.

Available platform plugins are: linuxfb, minimal, offscreen, vnc, xcb.

Aborted (core dumped)
connect: Connection refused
GKS: can't connect to GKS socket application

GKS: Open failed in routine OPEN_WS
GKS: GKS not in proper state. GKS must be either in the state WSOP or WSAC in routine ACTIVATE_WS

Update

After making sure that PyCall.jl and Conda.jl were properly configured I was able to execute the tests. The tests emit several warnings, and I would appreciate if the authors could discuss if these warnings are expected and how the user should respond to such warnings.

Test output with several warnings

``` Test Summary: | Pass Total Time numerical functions | 5 5 5.0s Test Summary: | Pass Total Time FFT + line fitting | 6 6 2.2s [ Info: New MicroTracker project created in /home/mkitti/review_temp/packages/MicroTracker/2n7hT/test/deletethis [ Info: ImageJ macro created at /home/mkitti/review_temp/packages/MicroTracker/2n7hT/test/deletethis/imagej_macro.ijm. See MicroTracker segmentation docs for instructions on how to use it. Test Summary: | Pass Total Time Project Creation | 7 7 1.4s Test Summary: | Pass Total Time Image loading and manipulation | 15 15 3.5s Test Summary: | Pass Total Time Python functions | 6 6 10.5s [ Info: Loading particle data for 5_13p5_61p35 [ Info: -> looks like ImageJ data, extracting frame column and making X Y lowercase. [ Info: adding info columns: ["B_mT", "FPS", "f_Hz"] [ Info: adding info columns: ["B_mT", "FPS", "f_Hz"] [ Info: Linking 5_13p5_61p35 ... ┌ Info: │ Done! 54 trajectories present. │ Filtered out stub trajectories < 0.5s resulting in 21 trajectories. └ ┌ Warning: Center point of trajectory 5_13p5_61p35-0 is out of bounds. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:254 ┌ Warning: Snipped trajectory from 5_13p5_61p35-0 is now too short. Deleting. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:296 ┌ Warning: Center point of trajectory 5_13p5_61p35-1 is out of bounds. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:254 ┌ Warning: Snipped trajectory from 5_13p5_61p35-1 is now too short. Deleting. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:296 ┌ Warning: Center point of trajectory 5_13p5_61p35-2 is out of bounds. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:254 ┌ Warning: Snipped trajectory from 5_13p5_61p35-2 is now too short. Deleting. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:296 [ Info: Loading particle data for 5_13p5_61p35 [ Info: -> looks like ImageJ data, extracting frame column and making X Y lowercase. [ Info: adding info columns: ["B_mT", "FPS", "f_Hz"] [ Info: Linking 5_13p5_61p35 ... ┌ Info: │ Done! 54 trajectories present. │ Filtered out stub trajectories < 0.5s resulting in 21 trajectories. └ ┌ Warning: Center point of trajectory 5_13p5_61p35-0 is out of bounds. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:254 ┌ Warning: Snipped trajectory from 5_13p5_61p35-0 is now too short. Deleting. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:296 ┌ Warning: Center point of trajectory 5_13p5_61p35-1 is out of bounds. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:254 ┌ Warning: Snipped trajectory from 5_13p5_61p35-1 is now too short. Deleting. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:296 ┌ Warning: Center point of trajectory 5_13p5_61p35-2 is out of bounds. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:254 ┌ Warning: Snipped trajectory from 5_13p5_61p35-2 is now too short. Deleting. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:296 [ Info: Loading particle data for 5_13p5_61p35 [ Info: -> looks like ImageJ data, extracting frame column and making X Y lowercase. [ Info: adding info columns: ["B_mT", "FPS", "f_Hz"] [ Info: Loading particle data for 5_13p5_61p35 [ Info: -> looks like ImageJ data, extracting frame column and making X Y lowercase. [ Info: adding info columns: ["B_mT", "FPS", "f_Hz"] [ Info: Linking 5_13p5_61p35 ... ┌ Info: │ Done! 54 trajectories present. │ Filtered out stub trajectories < 0.5s resulting in 21 trajectories. └ ┌ Warning: Center point of trajectory 5_13p5_61p35-0 is out of bounds. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:254 ┌ Warning: Snipped trajectory from 5_13p5_61p35-0 is now too short. Deleting. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:296 ┌ Warning: Center point of trajectory 5_13p5_61p35-1 is out of bounds. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:254 ┌ Warning: Snipped trajectory from 5_13p5_61p35-1 is now too short. Deleting. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:296 ┌ Warning: Center point of trajectory 5_13p5_61p35-2 is out of bounds. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:254 ┌ Warning: Snipped trajectory from 5_13p5_61p35-2 is now too short. Deleting. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:296 [ Info: Loading particle data for 5_8p4_28p68 [ Info: -> looks like ImageJ data, extracting frame column and making X Y lowercase. [ Info: adding info columns: ["B_mT", "FPS", "f_Hz"] [ Info: Linking 5_8p4_28p68 ... ┌ Info: │ Done! 18 trajectories present. │ Filtered out stub trajectories < 0.5s resulting in 16 trajectories. └ Test Summary: | Pass Total Time Particle data to linked data | 18 18 16.2s ┌ Warning: Center point of trajectory test-1 is out of bounds. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:254 ┌ Warning: Center point of trajectory test-1 is out of bounds. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:254 ┌ Warning: Snipped trajectory from test-1 is now too short. Deleting. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/linked_data.jl:296 Test Summary: | Pass Total Time Trajectory clipping | 8 8 0.7s Test Summary: | Pass Total Time helper functions | 1 1 0.2s [ Info: filtered trajectories 35 -> 21 Test Summary: | Pass Total Time Collapsed data | 9 9 8.3s ┌ Warning: framenumber 500 is greater than the last frame in the data, 340. Using the last frame instead. └ @ MicroTracker ~/review_temp/packages/MicroTracker/2n7hT/src/plotting.jl:97 [ Info: Creating animation: Frame = 120 qt.qpa.plugin: Could not find the Qt platform plugin "wayland" in "" ┌ Warning: ffmpeg() is deprecated, use the non-do-block form │ caller = ip:0x0 └ @ Core :-1 [ Info: Saved animation to /home/mkitti/review_temp/packages/MicroTracker/2n7hT/assets/test_animation.mp4 Test Summary: | Pass Total Time plotting | 7 7 17.5s Testing MicroTracker tests passed ```

mkitti commented 1 year ago

State of the Field

The paper mentions several other tracking packages written for other languages including Python and Java. It also happens to be able to integrate with some of them.

There is another Julia tracking package called BlobTracking.jl. Please compare and differentiate this package from BlobTracking.jl.

Kevin-Mattheus-Moerman commented 1 year ago

@czimm79 Note the above comments by one of the reviewers that need your attention ☝️

mkitti commented 1 year ago

Current review summary:

MicroTracker.jl is well packaged and documented. The package is used to track microbots, a form of active matter inspired by natural phenomena. While bespoke for a particular materials science experimental design, the package seems to have potential for general tracking problems, greatly expanding the number of potential users.

I appreciate that the authors have taken the time to mention some variety of other tracking packages. However, the tracking field is quite large with numerous tracking packages available. Because of this, I feel the authors have understated the "State of the Field". In particular several reviews cover the results of the particle tracking and cell tracking challenges. Mentioning the existence of these challenges would be helpful in expanding the discussion of the field.

In the expanded context, the authors should motivate further the creation of their software and in particular describe what makes microbots different from particles or cells. The authors currently that "MicroTracker.jl allows for size, shape, and rotation rate tracking of microbots which regularly enter and leave the video frame". However, it is unclear to me how that differs from other tracking problems that appear in the previously mentioned challenges.

What would help is to first explain the nature of the problem that this package is meant to address. In particular, what are the unique challenges that tracking microbots present? What are the unique or particular solutions that this package implements? Some hint of this is given in the paragraph starting on line 48, but this needs to be expanded and probably should some much earlier in the paper.

There are a few technical points that I've outlined in detail above.

This Julia package has Python package dependencies. There is no mention of this in the Quick Start or Setup portions of the documentation. However, the interface between Python and Julia often needs careful configuration. In particular, the authors use the function pyimport_conda from PyCall.jl. Proper use of this function requires that both the packages Conda.jl and PyCall.jl be consistently configured. The authors should at minimum note the use of these dependencies and reference the documentation for those packages.

The versions of the Julia and Python dependencies should be listed for reproducibility.

Automated testing does exist in the form of Julia tests and is run by Github Actions on each commit. This obviously only works with a properly installed package including with the Python dependencies.

Additionally, the community guidelines requirement of this journal are not fully met as per the above.

Overall, MicroTracker.jl is a very promising package in a fastastic state. Some improvements could be made. In particular, explaining the problem being solved by this package thoroughly will help potential users evaluate if they have similar problems that could be solved either by this package directly or could be solved by studing the open source code of this package. Some technical improvements could be made or documented to help users forestall potential issues. I believe these issues could be addressed by the authors in a reasonable amount of time, and I look forward to seeing those changes.

mkitti commented 1 year ago

Some technical challenges I had to overcome to properly configure the Julia - Python interface:

PyCall.jl initially detected the system python3 installed in Ubuntu rather than the python executable installed by conda in Conda.jl. To properly configure this, I needed to either remove python3 from my executable path or configure my executable path to be of the conda environment followed by using Pkg.build to rebuild PyCall.jl.
Setting the $PYTHON environment variable alone was not compatible with the author's use of pyimport_conda since in this mode PyCall.jl is not aware of the conda installation. In this case, Conda.add needed to be invoked manually before pyimport_conda could succeed.

I believe the author's current installation process may succeed in the conditions where there is no python executable on the current path, PyCall.jl uses Python from the Conda.jl conda install, and then configures itself to use both the executables python and conda from the same environment.

It is not the author's responsibility to solve and lessen this complex process, but it would helpful to note that this is part of the installation process and where to get help if the above process goes awry as it did for me on my Windows and Linux machines. Fortunately, I have some expertise in this matter and was able to resolve them. It would be helpful if the author's could point other potential users to the PyCall.jl and Conda.jl documentation for troubleshooting.

BioTurboNick commented 1 year ago

Review checklist for @BioTurboNick

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the https://github.com/czimm79/MicroTracker.jl?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@czimm79) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
[x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
[x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
[x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

BioTurboNick commented 1 year ago

I was unable to install the code as directed here (https://czimm79.github.io/MicroTracker.jl/dev/quickstart/), thanks to a Python/Conda issue, documented here: https://github.com/czimm79/MicroTracker.jl/issues/40

UPDATE: I got it to install as expected on a different machine.

BioTurboNick commented 1 year ago

Noted that the summary seems incomplete, and contains jargon https://github.com/czimm79/MicroTracker.jl/issues/41

BioTurboNick commented 1 year ago

Noted that the Statement of Need seems to be overloaded, advised to move the implementation details into a different section: https://github.com/czimm79/MicroTracker.jl/issues/42

BioTurboNick commented 1 year ago

I think the documentation's statement of need could be expanded placed in the formal docs, not just in the README: https://github.com/czimm79/MicroTracker.jl/issues/43

BioTurboNick commented 1 year ago

Also I generally concur with @mkitti's points.

czimm79 commented 1 year ago

Thank you all for your review and insightful comments on our work. We will begin to address and implement the solutions discussed here. When ready for review, I will create a PR which addresses each issue and link to them from this thread.

mkitti commented 1 year ago

Thank you. This review format allows for an interactive conversation, so I would be happy to discuss anything about it in the meantime. I look forward to seeing your revisions.

Kevin-Mattheus-Moerman commented 1 year ago

@czimm79 hope you are getting one well. Would you be able to provide an update on how you are addressing the above?

czimm79 commented 1 year ago

@Kevin-Mattheus-Moerman Yes, I would be happy to provide an update on our remaining blockers by the end of this week. Thank you all for your patience, I was without a workstation during my recent move.

czimm79 commented 1 year ago

As promised, here is the current status on the review.

The majority of code revisions are complete, and are ready for review in the three PRs referenced above.
Manuscript revisions are nearing completion. I expect to share the new draft next week, along with responses to each reviewer comment regarding content.
The transfer of the example videos from the package repository to the Julia Artifacts system is in progress. I'm currently working on making sure the artifact isn't downloaded multiple times, as the example videos are used both during the CI tests and the quick start guide. I expect this to be finished late next week.

Thank you all again for your patience and helpful comments.

Kevin-Mattheus-Moerman commented 1 year ago

@czimm79 thanks for the updates/links. Let me know if I should tell the reviewers to pick this up again, or if you are still working on things.

czimm79 commented 1 year ago

Hi @Kevin-Mattheus-Moerman - Still working on things, the response should be complete by the end of this week. Thanks.

czimm79 commented 1 year ago

@Kevin-Mattheus-Moerman The response is ready for the reviewers to pick up again.

Reviewers: Please find the responses in the MicroTracker pull requests tab. After the review of each PR, the plan is to merge them into the review branch. Then, after verifying everything works well combined, the review branch will be merged into master with a new version increment.

Please let me know if I missed anything. I will be monitoring this thread and the PRs for any continued discussion. Thank you.

Kevin-Mattheus-Moerman commented 12 months ago

@roflmaostc, @mkitti, @BioTurboNick please pick up the review and provide further feedback to the authors where needed. Thanks!

Kevin-Mattheus-Moerman commented 11 months ago

@roflmaostc, @mkitti, @BioTurboNick please pick up the review and provide further feedback to the authors where needed. Thanks!

mkitti commented 11 months ago

My remaining comments concern the Python interface as detailed in https://github.com/czimm79/MicroTracker.jl/pull/47#pullrequestreview-1787821495 .

It appears that PyCall currently does not support non-root Conda environments

There is some misreading of the documentation here. Conda.ROOTENV is the default value for the optional env arguments across the Conda.jl API. However, you can specify the environment by passing a second argument to functions such as Conda.add and Conda.add_channel.

PyCall.jl can be used with non-root environments by

Setting the environment variable PYTHON either via ENV["PYTHON"] in Julia or in the shell before starting Julia.
Calling using Pkg; Pkg.build("PyCall") as per the PyCall.jl README.

BioTurboNick commented 11 months ago

Mark's comments are good, but otherwise LGTM!

roflmaostc commented 11 months ago

LGTM too!

czimm79 commented 11 months ago

Thanks all. I plan on addressing @mkitti's comments by the end of this week.

czimm79 commented 10 months ago

Hi @Kevin-Mattheus-Moerman - I believe we're ready to move forward. I've worked with Mark to address his comments over on https://github.com/czimm79/MicroTracker.jl/pull/47. The combined reviewer comments have been pushed to the master branch and tagged with v0.3. Let me know what I need to do next. Thanks!

mkitti commented 10 months ago

@editorialbot generate pdf

editorialbot commented 10 months ago

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

mkitti commented 10 months ago

@BioTurboNick , could you check if you the remaining checklist items have been satisfied?

BioTurboNick commented 10 months ago

Done

Kevin-Mattheus-Moerman commented 10 months ago

@mkitti @BioTurboNick @roflmaostc thanks for your excellent reviews! All boxes are now ticked. It looks like @BioTurboNick @roflmaostc have already given their blessing. @mkitti at this point would you recommend acceptance in JOSS as well? Thanks!

mkitti commented 10 months ago

Yes, I recommend acceptance.

Kevin-Mattheus-Moerman commented 10 months ago

@czimm79 it looks like all reviewers have now given their blessing. So we can proceed to process this work for acceptance in JOSS.

Kevin-Mattheus-Moerman commented 10 months ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

[x] Double check authors and affiliations (including ORCIDs)
[x] Make a release of the software with the latest changes from the review and post the version number here. This is the version that will be used in the JOSS paper.
[x] Archive the release on Zenodo/figshare/etc and post the DOI here.
[x] Make sure that the title and author list (including ORCIDs) in the archive match those in the JOSS paper.
[x] Make sure that the license listed for the archive is the same as the software license.

Editor Tasks Prior to Acceptance

[x] Read the text of the paper and offer comments/corrections (as either a list or a PR)
[x] Check the references in the paper for corrections (e.g. capitalization)
[x] Check that the archive title, author list, version tag, and the license are correct
[x] Set archive DOI with @editorialbot set <DOI here> as archive
[x] Set version with @editorialbot set <version here> as version
[x] Double check rendering of paper with @editorialbot generate pdf
[x] Specifically check the references with @editorialbot check references and ask author(s) to update as needed
[ ] Recommend acceptance with @editorialbot recommend-accept

Kevin-Mattheus-Moerman commented 10 months ago

@czimm79 can you work on your set of points here ☝️

In addition:

[ ] Please add a References heading prior to the reference list.
[ ] Please spell out USA as United States of America
[ ] The second affiliation contains |, can you reformat this? This should perhaps be a comma instead.
[ ] Check the potentially invalid DOI's

Kevin-Mattheus-Moerman commented 10 months ago

@editorialbot generate pdf

openjournals / joss-reviews

[REVIEW]: MicroTracker.jl: A Julia package for microbot research #5804

Status

Reviewer instructions & questions

Checklists

Review checklist for @roflmaostc

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @mkitti

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Installation

Reproducibility

Community Guidelines

Automated testing

Update

State of the Field

Review checklist for @BioTurboNick

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance