[REVIEW]: instrbuilder: a Python package for control of electrical instruments

whedon commented 5 years ago

Submitting author: @lucask07 (Lucas J. Koerner) Repository: https://github.com/lucask07/instrbuilder Version: v0.1.9 Editor: @arokem Reviewer: @brainstorm, @casteer Archive: 10.5281/zenodo.2640871

Status

Status badge code:

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

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) 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

@brainstorm, @casteer, please carry out your reviews in this issue by updating the checklist below. If you cannot edit the checklist please:

Make sure you're logged in to your GitHub account
Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @arokem know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @brainstorm

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

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 repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Version: v0.1.9
[x] Authorship: Has the submitting author (@lucask07) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

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 function 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] Authors: Does the paper.md file include a list of authors with their affiliations?
[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] References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @casteer

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

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 repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Version: v0.1.9
[x] Authorship: Has the submitting author (@lucask07) 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

[x] 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.
[x] 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?
[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] Authors: Does the paper.md file include a list of authors with their affiliations?
[x] 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 commented 5 years ago

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

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

Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

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 commented 5 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

brainstorm commented 5 years ago

@lucask07 It would help to have some pointers on installing the (generic?) VISA drivers. I own a Rigol MSO5000, but VISA from NI downloads section doesn't lead to any result actually. More hints/pointers would be welcome!

Also, when installing via pip install instrbuilder, how is the user supposed to search for the examples you point out? Those small UX details can help saving some time for your users, IMHO.

lucask07 commented 5 years ago

Hi @brainstorm, Thank you for the feedback! I will:

Update the NI VISA link in the readme (this page is my intention http://www.ni.com/en-us/support/downloads/drivers/download.ni-visa.html)
Create a pointer to the file location of the examples: (/$PATH_TO_PYTHON_INSTALL/lib/python3.7/site-packages/instrbuilder/examples/).

where $PATH_TO_PYTHON_INSTALL can be found with which python

I have a Rigol scope and will test with that and should be done by the end of the week. What OS are you running?

brainstorm commented 5 years ago

Thanks much Lucas,

I'm using OSX at the moment.

From what I can see on your requirements.txt, you are installing a ophyd fork already, right? Are the installation steps in the README matching current reality? From the looks of it, a simple pip install instrbuilder should suffice after installing the VISA drivers? :)

lucask07 commented 5 years ago

@brainstorm I've updated the pip package to version 0.1.5 and improved the README.

The update includes commands.csv, an instrument class, and an example for a Rigol oscilloscope. Please see a much-updated README for more complete install instructions and pointers to the files you asked about above. I tested the Rigol scope example out, it would be great if it works on your end too!

The requirements.txt file was not successfully forcing the ophyd fork installation from GitHub. My research suggests that PyPi does not allow remote URL dependencies. See the Ophyd fork install issue that I opened in the instrbuilder repository. Since ophyd/bluesky is a bonus add on I have:

removed ophyd from the requirements.txt
added try/except around any from ophyd.ee_instruments import ... to let the user know the need to install the ophyd fork for GitHub.
Moved the Bluesky/ophyd documentation to the bottom of the README to limit confusion.

brainstorm commented 5 years ago

Fantastic, thanks much @lucask07 for the detailed README instructions, now I managed to get some good output:

$ ./rigol_mso5000_fetch.py
Error:  <class 'ImportError'>
IC (integrated circuit imports failed)
The aardvark.so or dll must be in the cwd or an importable path
Continuing anyways, since many may not use this portion...
Opened Instrument: RIGOL TECHNOLOGIES,MSO5072,SN,00.01.01.02.03
Saving file: rigol_mso5000_screenshot.png
Measured pk-pk voltage of = 0.217338
Measured avg voltage of = -0.03870067

Now, other than some polishing on the "safe-to-ignore" Errors (which you raise in the README), I cannot check off the testing part. I know that testing with this type of very specialized hardware is hard (no pun intended), but have you considered mock testing for the instruments for which you know the inputs/outputs of?

A small test (that covers simulated functionality) as a proof of concept should be enough to tick that box off, kudos for this software effort! 👍

casteer commented 5 years ago

Hi @lucask07, have you got an advice and/or pointers on how to install the NI VISA drivers for linux distributions other the offically-supported ones? There's an reference to this being possible but cumbersome in the PyVISA Installation docs. I'm using Ubuntu 18.04 and the best I could I find is an unhelpful note... . I'll see how much I can do though without this in the meantime. Thanks in advance for any help that you're able to give me!

lucask07 commented 5 years ago

@casteer Apologies, I have not tried to install NI-VISA on an Ubuntu system. An alternative is the pure Python VISA backend pyvisa-py. This can be installed via pip. If both NI-VISA and pyvisa-py are installed PyVISA defaults to the NI backend. To activate this alternate backend you pass to the VISA resource manager:

visa.ResourceManager('@py')

See more information on configuring the backend to PyVISA here: [pyvisa backend info](https://pyvisa.readthedocs.io/en/master/configuring.html

If you don't have NI-VISA installed PyVISA will default to the pyvisa-py backend and this may all work. If you have both installed and you want to use pyvisa-py I will need to make a few simple changes to instrbuilder.

Let me know how it goes.

lucask07 commented 5 years ago

@brainstorm Thank you for the feedback. For testing, I have added an "unconnected" property to instruments and am using an unconnected instrument to test functionality.

My testing is using the pytests package.

The script to run tests is: run_tests.py And the tests are at tests/test_command.py

8 tests are run, 8 tests pass.
Code coverage on commands.py is 79%
Code coverage on scpi.py is 45%

Neither is great, but its a start! and I'm seeing why the tests help development.

lucask07 commented 5 years ago

@arokem The package has been updated to version 0.1.6 on PyPi and github. I presume we can update the JOSS version to match what is in github (once all changes are done)?

brainstorm commented 5 years ago

@whedon check references

whedon commented 5 years ago

Attempting to check references...

whedon commented 5 years ago


OK DOIs

- http://doi.org/10.1080/08940886.2017.1289810 is OK

MISSING DOIs

- None

INVALID DOIs

- None

arokem commented 5 years ago

@lucask07 : regarding versions: yes, that's not a problem. The eventual paper will point to a DOI of an archive that you will create (e.g., on zenodo) from the version of the software that incorporates comments/fixes that happened through the review process.

lucask07 commented 5 years ago

Hi @casteer, I researched and tested a way to get this up and running with Ubuntu. With Ubuntu, we can use the pure Python VISA back end. This works on my system using a function generator. See the detailed setup instructions which I've added to the instrbuilder documentation: Visa with Linux

lucask07 commented 5 years ago

Hi @casteer, Any luck or any questions with the Linux setup?

casteer commented 5 years ago

Hi, Thanks so much @lucask07 for the new help page and I'm sorry for the delay. I've got it install and will be instrument testing over the next couple of days. Best wishes.

arokem commented 5 years ago

Hi @casteer : any progress?

brainstorm commented 5 years ago

@arokem If that helps, it does tick all the boxes for me 👍

arokem commented 5 years ago

Thanks @brainstorm !

@casteer : any progress on your review?

arokem commented 5 years ago

@whedon render pdf

arokem commented 5 years ago

@whedon commands

whedon commented 5 years ago

Here are some things you can ask me to do:

# List all of Whedon's capabilities
@whedon commands

# Assign a GitHub user as the sole reviewer of this submission
@whedon assign @username as reviewer

# Add a GitHub user to the reviewers of this submission
@whedon add @username as reviewer

# Remove a GitHub user from the reviewers of this submission
@whedon remove @username as reviewer

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

# Change editorial assignment
@whedon assign @username as editor

# Set the software archive DOI at the top of the issue e.g.
@whedon set 10.0000/zenodo.00000 as archive

# Set the software version at the top of the issue e.g.
@whedon set v1.0.1 as version

# Open the review issue
@whedon start review

EDITORIAL TASKS

# Compile the paper
@whedon generate pdf

# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name

# Remind an author or reviewer to return to a review after a
# certain period of time (supported units days and weeks)
@whedon remind @reviewer in 2 weeks

# Ask Whedon to accept the paper and deposit with Crossref
@whedon accept

# Ask Whedon to check the references for missing DOIs
@whedon check references

arokem commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

arokem commented 5 years ago

@lucask07 : I apologize for the delay here. As @brainstorm has given this the 👍 and @casteer seems to have disappeared, I am inclined to move ahead with your article. I have the following comments:

The authors in the bibliography sometimes don't make sense. For PyVISA, please ask the maintainers how to refer to the software (maybe they should publish in JOSS...). For the SCPI syntax and style guide, I believe that no author should be named.
We might need a reference for "Aardvark controller" (e.g., who manufactures this and where?).
Is there already a reference for the "in press" article mentioned in the last paragraph?

If you could address these comments, we can move ahead with the process of accepting your article for publication. Thanks for your patience!

lucask07 commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

lucask07 commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

lucask07 commented 5 years ago

@arokem : Thank you for your comments.

I've updated the PyVISA author entry in the Bibtex so that it is not abbreviated. It is sensible now. I've also added an issue to the PyVISA repository to see if there is a preferred citation. For the SCPI style guide, I also fixed the abbreviation. However, the author is not empty since that would break the (author, year) citation format in the text.
The text now reads Aardvark controller (Total Phase, Santa Clara, CA).
There is not yet a citation available for this article. Instead, I've added a link to a MWE that uses both instrbuilder and the NSLS-II Bluesky suite.
Small grammatical changes and modifications to eliminate double parantheses (due to the citations).

Let me know if this seems OK.

lucask07 commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

lucask07 commented 5 years ago

In response to https://github.com/pyvisa/pyvisa/issues/407 switched to short links for both PyVISA and PySerial.

arokem commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

arokem commented 5 years ago

@whedon check references

whedon commented 5 years ago

Attempting to check references...

whedon commented 5 years ago


OK DOIs

- 10.1080/08940886.2017.1289810 is OK

MISSING DOIs

- None

INVALID DOIs

- None

arokem commented 5 years ago

This all looks good to me. Could you please create an archive of the software as it is now (e.g., using zenodo) and post the DOI here?

lucask07 commented 5 years ago

@whedon set 10.5281/zenodo.2640871 as archive

whedon commented 5 years ago

I'm sorry @lucask07, I'm afraid I can't do that. That's something only editors are allowed to do.

lucask07 commented 5 years ago

@arokem I've pulled a DOI from zenodo: 10.5281/zenodo.2640871

openjournals / joss-reviews