Skip to content
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]: labjack-controller: Fast Realtime Data Collection with Labjack T-Series DAQs in Python #1448

Closed
whedon opened this issue May 14, 2019 · 48 comments
Assignees
Labels

Comments

@whedon
Copy link
Collaborator

@whedon whedon commented May 14, 2019

Submitting author: @Nyctanthous (Benjamin Montgomery)
Repository: https://github.com/university-of-southern-maine-physics/labjack-controller
Version: 0.4.1
Editor: @usethedata
Reviewer: @lucask07, @ixjlyons
Archive: 10.5281/zenodo.3247140

Status

status

Status badge code:

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

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

@lucask07 & @ixjlyons, 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 @usethedata know.

Please try and complete your review in the next two weeks

Review checklist for @lucask07

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: 0.4.1
  • Authorship: Has the submitting author (@Nyctanthous) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • 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

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • 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

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @ixjlyons

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: 0.4.1
  • Authorship: Has the submitting author (@Nyctanthous) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • 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

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • 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

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?
@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented May 14, 2019

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @lucask07, it looks like you're currently assigned as the reviewer for this paper 🎉.

⭐️ Important ⭐️

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
@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented May 14, 2019

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented May 14, 2019

@labarba

This comment has been minimized.

Copy link
Member

@labarba labarba commented May 14, 2019

👋 @lucask07, @ixjlyons — Thanks for agreeing to review this submission for JOSS! This is where it happens. Follow your review checklist above, and feel free to ask questions here or post issues in the submission repository.

@ixjlyons

This comment has been minimized.

Copy link

@ixjlyons ixjlyons commented May 28, 2019

My review:

labjack-controller is a Python library which aims to hide some of the low-level details of collecting data from a LabJack device. The interface is well thought-out and the documentation contains enough information to get started and make full use of the library. The code is very clean and seems to be written well with good error handling (and nice use of type hints!). The paper presents both the motivations for the library and the research purpose clearly.

There were a minor issues I noticed in the documentation:

  • Overview page:
    • Requirements section:
      • In the note on LJM, typo: "The LJM library is provided a a..."
    • Installation section:
      • The installation steps miss pip install .. It's mentioned below but I think the note below should say you can omit pip install -r requirements.txt if you already have LJM installed.
      • There is no requirements.txt (use the one in docs/?)
  • Demo notebook 1
    • Step 1 section
      • Typo "...there will be a notably offset..."
      • Typo "Additionally, if you are perofrming..."
  • API Documentation
    • There's an empty "Module contents" section at the bottom
    • LabJackReader.collect_data example says 50kHz sample rate but shows 10 kHz.
  • Contributing guidelines are missing (part of community guidelines).

And on to a couple bigger issues. After reading the docs through, it's still not perfectly clear to me the interaction between scan_rate and scans_per_read. From the descriptions given, scan_rate sounds like sample rate (i.e. how rapidly the LabJack actually samples the analog inputs or effectively does so [i.e. if it supersamples internally, not sure]), and scans_per_read sounds like it controls the number of samples returned on each update from the device. If this is the case, consider using "update rate," "read rate," or maybe "frame rate" instead of "sample rate" in the scans_per_read docstring -- "sample rate" to me usually indicates the rate of sampling the analog signal.

The use of the term "realtime" to me implies the library is set up to continuously stream data, such as reading a frame of samples at a time and continuously updating a live plot with the collected data, say, every second. Looking at the documentation and code, it seems the library is more for doing what I would maybe call "data logging." The only LabjackReader method for recording data is collect_data, which opens and closes the stream on each call, so it doesn't look to me like it can be used for continuous streaming where the user needs to periodically work with the data being streamed. This could be implemented with a callback mechanism where the user can pass a callback function to collect_data which receives each frame as it comes in, for example. Currently, the user could work with LJMLibrary to achieve this, but this is fairly low-level functionality which I think labjack-controller aims to hide. I don't think it's necessary to support this kind of functionality for the library to be useful, but after reading the paper, I'd expect that capability to be implemented. I would recommend that you either implement an interface for continuous streaming or make it clear in the paper/docs that the main functionality is collecting data for a specific amount of time.

Overall, labjack-controller is a well-written library and I think it has sufficient general scientific research purpose to fit in to JOSS. I recommend accepting so long as the notes above are taken into consideration.

@Nyctanthous

This comment has been minimized.

Copy link

@Nyctanthous Nyctanthous commented May 28, 2019

@ixjlyons Thank you, you caught some things that totally slipped by me. All of the minor issues you mentioned should be fixed in the next 24 hours or so.

Adding a callback as a kwarg is trivial; I'll look into a solution to make the realtime bit more accessible. Currently, realtime functionality requires parallel processing; the Labjack vomits data all over the place, as soon as you start streaming and if you spend too much time in between acquiring frames, it will overflow all internal buffers and the stream will die. I expect I can hide this issue from the user by maintaining a thread pool and spinning off new callbacks from the pool.

I'll also clear up the scans_per_read / scan_rate confusion in the documentation and code. scan_rate is sampling frequency, scans_per_read represents the limitations of inter-device communication; when you send information from the labjack to your computer, it does it in batches, where each "scan" is a complete row of data. scans_per_read determines this batch size.

I expect to have everything fixed up by this weekend.

@lucask07

This comment has been minimized.

Copy link

@lucask07 lucask07 commented May 29, 2019

This is a very nicely written software package and certainly fills a need. I'm a LabJack user and realize that the cost and ease of use (as compared to say, an NiDAQ) is great. Keep up the nice work!

Review checkboxes that are yet to be ticked:

  • Community Guidelines Missing a note on how to ask for support
  • References in the paper are all cited as "n.d." This should be fixed somehow.
  • Functionality: Have the functional claims of the software been confirmed? need clarification or rewording as it relates to 'realtime'. Or a demonstration (with a function generator) that realtime is achieved.

Other Notes:

Unfortunately, I have a U6 LabJack (not a T series) so am not able to test this package with actual hardware. Note that for the U series LabJack has published a Python API: U series Python API.

The title of the paper does not seem to accurately reflect what the software provides.

  • By "Fast" do you mean:
  1. that the user can get the software up and running quickly?
  2. or that the data transfer rate to the host computer is maximized in some way?
  • "Realtime" is a very specific term and often implies that data is sampled at clock rates entirely set by hardware and no (non-realtime) operating system is involved. I suspect with this software the data acquired is in effect "realtime" assuming that only one LabJack to host data transfer occurs. If, however, multiple transfers occur there will be indeterminate gaps in time set by the latency of the LabJack to host transfer. A way around this so that this package continuously streams data at an exact sampling rate would be a great addition. Alternatively, a demonstration with a sine-wave function generator input (at a frequency of ~200 Hz) may help explain to the user:
  1. the number of sampling points that can be retrieved with no gaps and
  2. the number of sampling points that produce a gap in the time sequence and the size of that gap.

A few other notes, ideas, and questions

  • The paper references "real-time reaction to sensor readings". I don't follow how this is happening. Again, to me, "real-time" implies no interaction with a non-realtime operating system. Please explain or remove from the paper.

  • I'm noting some confusion with the setting mode of analog streaming versus the settling time. I see both of these variables and, at times, one may be mixed up for the other:
    stream_setting versus stream_settling

For example, within the docstring for _setup

        stream_settling: int, optional
            See official LabJack documentation.

I'd expect that a settling time would be a float. Could you please check this? And add to the documentation the streaming mode(s) that you intend to support?

  • Is there a plan to implement an interface to the digital I/O?
@Nyctanthous

This comment has been minimized.

Copy link

@Nyctanthous Nyctanthous commented Jun 3, 2019

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jun 3, 2019

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jun 3, 2019

PDF failed to compile for issue #1448 with the following error:

Error reading bibliography ./paper.bib (line 21, column 3):
unexpected "m"
expecting space, ",", white space or "}"
Error running filter pandoc-citeproc:
Filter returned error status 1
Looks like we failed to compile the PDF

@Nyctanthous

This comment has been minimized.

Copy link

@Nyctanthous Nyctanthous commented Jun 3, 2019

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jun 3, 2019

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jun 3, 2019

@Nyctanthous

This comment has been minimized.

Copy link

@Nyctanthous Nyctanthous commented Jun 3, 2019

@lucask07 Thanks for your review. I believe that, at this point, I have addressed everything you pointed our as incorrect. A few comments explaining what my thinking was at the time:

  1. Usage of the word "Realtime". The T4, and T7 are (probably) realtime in the sense that you indicate. However, this package makes no effort to propagate this attribute through the process of TCP or UDP data streaming. At some future point, I agree with you that it would be very neat to add, but I expect that will take a while to develop and perfect. When I used "realtime", I meant it to act like the word "live"; e.g. "live data processing". That has since been corrected, and "realtime" has been completely removed.
  2. Usage of the word "Fast". Anecdotally, the package is faster than a one-to-one Python wrapper of the C functions due to a reduction of overhead; the package is set up in a way that we minimize type conversion, and rely heavily on primitive (ctypes, numpy) types in the first place. With that said, I have removed these claims because they are difficult to quantify - any benefit gets overshadowed by the cost of opening/closing/communicating with the LabJack.
  3. stream_setting vs stream_settling: The "Setting" option does not exist (surprise, surprise), and was solely exists as a typo. Any actual stream setting out there (there aren't that many) you can set with a Labjack object's modify_setting function. This has also been cleared up.
  4. Interfacing the digital I/O. We already do! You can stream any 16 bit channel; the process is almost exactly the same. I've added examples to the demo files and the documentation.
  5. "n.d." references. This stands for "No Date", due to these references being websites with unclear authorship dates. To come up with a date, I looked at the date that the Python bindings for each of these references had been updated on.

@ixjlyons I think I have also addressed all of your points in the manner I outlined previously.

At this point, I expect we can continue the review and editing process.

@lucask07

This comment has been minimized.

Copy link

@lucask07 lucask07 commented Jun 5, 2019

@Nyctanthous looks great, thanks for the thorough response to my critique. Note that I cannot verify the functionality since I don't have the hardware. I now accept this submission. Nice work!

@usethedata

This comment has been minimized.

Copy link
Collaborator

@usethedata usethedata commented Jun 9, 2019

@lucask07 and @ixjlyons There are a couple of things still showing open on the review checklists. If all your issues are taken care of, can you close out the checklists? I realize that @lucask07 indicates not being able to complete a functional check. @ixjlyons -- were you able to do a functional check? If not, I'll see if I can find someone around my lab who has the hardware.

@ixjlyons

This comment has been minimized.

Copy link

@ixjlyons ixjlyons commented Jun 10, 2019

The changes look good to me and I checked off my remaining boxes. I don't have a LabJack device so I was not able to test the library myself.

@usethedata

This comment has been minimized.

Copy link
Collaborator

@usethedata usethedata commented Jun 16, 2019

@Nyctanthous This paper looks ready to go. Can you please generate an archive version? Post the archive DOI back in here and let me know the version of the software you archived. I'll double check things and update the archive and version here. Thanks.

@usethedata

This comment has been minimized.

Copy link
Collaborator

@usethedata usethedata commented Jun 16, 2019

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jun 16, 2019

Attempting PDF compilation. Reticulating splines etc...
@usethedata

This comment has been minimized.

Copy link
Collaborator

@usethedata usethedata commented Jun 16, 2019

@whedon check references

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jun 16, 2019

Attempting to check references...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jun 16, 2019

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jun 16, 2019


OK DOIs

- None

MISSING DOIs

- https://doi.org/10.1088/0950-7671/36/9/414 may be missing for title: Labjack

INVALID DOIs

- None
@usethedata

This comment has been minimized.

Copy link
Collaborator

@usethedata usethedata commented Jun 16, 2019

I confirm that the DOI which whedon suggests above is not relevant to this paper. It is a reference to a physical device, used to raise and lower laboratory equipment, and has nothing to do with the labjack software.

@Nyctanthous

This comment has been minimized.

Copy link

@Nyctanthous Nyctanthous commented Jun 16, 2019

@usethedata I created the release 0.4.1 to make Zenodo happy. DOI is 10.5281/zenodo.3247140

@usethedata

This comment has been minimized.

Copy link
Collaborator

@usethedata usethedata commented Jun 18, 2019

@whedon set 0.4.1 as version

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jun 18, 2019

OK. 0.4.1 is the version.

@usethedata

This comment has been minimized.

Copy link
Collaborator

@usethedata usethedata commented Jun 18, 2019

@whedon set 10.5281/zenodo.3247140 as archive

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jun 18, 2019

OK. 10.5281/zenodo.3247140 is the archive.

@usethedata

This comment has been minimized.

Copy link
Collaborator

@usethedata usethedata commented Jun 18, 2019

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jun 18, 2019

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jun 18, 2019

@usethedata usethedata added the accepted label Jun 18, 2019
@usethedata

This comment has been minimized.

Copy link
Collaborator

@usethedata usethedata commented Jun 18, 2019

@openjournals/jose-eics I believe this article is ready.

@danielskatz

This comment has been minimized.

Copy link

@danielskatz danielskatz commented Jun 18, 2019

@whedon accept

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jun 18, 2019

Attempting dry run of processing paper acceptance...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jun 18, 2019


OK DOIs

- None

MISSING DOIs

- https://doi.org/10.1088/0950-7671/36/9/414 may be missing for title: Labjack

INVALID DOIs

- None
@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jun 18, 2019

Check final proof 👉 openjournals/joss-papers#776

If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#776, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
@danielskatz

This comment has been minimized.

Copy link

@danielskatz danielskatz commented Jun 18, 2019

@whedon accept deposit=true

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jun 18, 2019

Doing it live! Attempting automated processing of paper acceptance...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jun 18, 2019

🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jun 18, 2019

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited 👉 openjournals/joss-papers#778
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.01448
  3. If everything looks good, then close this review issue.
  4. Party like you just published a paper! 🎉🌈🦄💃👻🤘

Any issues? notify your editorial technical team...

@danielskatz

This comment has been minimized.

Copy link

@danielskatz danielskatz commented Jun 18, 2019

Thanks to @lucask07 and @ixjlyons for reviewing and @usethedata for editing!

@danielskatz

This comment has been minimized.

Copy link

@danielskatz danielskatz commented Jun 18, 2019

👋 @lucask07 - I note one checkbox that you didn't check - can you please do that before I close this review/issue?

(If there's a problem, we can unpublish the paper - sorry I didn't see this until the last minute)

@lucask07

This comment has been minimized.

Copy link

@lucask07 lucask07 commented Jun 18, 2019

Hi @danielskatz
I am not able to check the functionality box because I do not have the hardware (T-series Labjack). I reviewed the code and confirmed the installation procedure. I'd suggest the paper remains published even without that checkbox.

@danielskatz

This comment has been minimized.

Copy link

@danielskatz danielskatz commented Jun 18, 2019

Ok, thanks - given that the other reviewer (@ixjlyons) did check this box, we'll go ahead

@whedon

This comment has been minimized.

Copy link
Collaborator Author

@whedon whedon commented Jun 18, 2019

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](http://joss.theoj.org/papers/10.21105/joss.01448/status.svg)](https://doi.org/10.21105/joss.01448)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.01448">
  <img src="http://joss.theoj.org/papers/10.21105/joss.01448/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: http://joss.theoj.org/papers/10.21105/joss.01448/status.svg
   :target: https://doi.org/10.21105/joss.01448

This is how it will look in your documentation:

DOI

We need your help!

Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

@Nyctanthous

This comment has been minimized.

Copy link

@Nyctanthous Nyctanthous commented Jun 18, 2019

Thank you all so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.