-
-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[REVIEW]: Hardware-Control: Instrument Control and Automation Package #2688
Comments
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @aquilesC, @untzag, @garrettj403 it looks like you're currently assigned to review this paper 🎉. Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post. ⭐ 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:
For a list of things I can do to help you, just type:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
|
@timtroendle, I did a quick check of the code and there are few things to check/improve, which means the package is not yet ready to be published. I also noticed that the latest commit to the master branch was only 11 hours ago, so the code is still evolving and this means it is a moving target for reviewing. I am not sure how should I proceed? |
Thanks @aquilesC for pointing this out. @Grant-Giesbrecht, can you please make sure there is a stable version of the package which can be reviewed? You stated the version for this submission to be "v1.0.0", but this version does not exist in the repo. Can you please add it? You can of course continue the development, but these changes should remain outside the scope of this submission. |
Hello, I'm one of the co-authors. We do have a 1.0.0 released and that should be the version to be reviewed. The released version can be found on pypi https://pypi.org/project/hardware-control/. I noticed that we forgot to push the tag to bitbucket, I just fixed that... thanks for pointing this out. |
Dear @arunpersaud , thanks for clarifying! So I looked at the v1.0.0. First, I would like to acknowledge your hard work on a topic that is normally ill defined, such as instrumentation with Python. I liked some of your approaches, such as including a terminal within the UI to gain extra control of the devices without leaving it. I also appreciate the effort for defining reusable user interfaces. I, personally, don't like the idea of defining your own commands to control instruments, but I understand why you do it. However, this requires a lot of good documentation and justification, which is currently lacking. As reviewers, we have to follow a checklist to justify our decision (you can see them above), and sadly I do not consider this work to be ready to be published. However, I took the time to make an extensive justification of the things that can be improved, perhaps for a future version 1.1.0. Some of the topics are structural, and therefore I don't think it is wise to open individual issues for each one of my thoughts. (Disclaimer: I have been working on and teaching Python for Instrumentation for almost 10 years now, so I may put the bar a bit higher and use this review as a teaching and learning opportunity as well, so don't take the remarks personally but judge them rationally and try to see what things make sense and can be improved). There are other reviewers and an editor involved, and our opinions may be different. I do think your program has potential, and it helps boost the image of Python as a valid alternative in the particle physics experimental community. Paper
Program
Documentation
|
Thanks @aquilesC for the detailed review. A lot to go through ;) I agree with most points and will have to look more into some of the others. Also thanks for pointing out some of the other libraries. I wasn't aware of PyMeasure and that looks very interesting. For a few points I probably have some followup questions and was wondering if you would be available to discuss them (perhaps offline to not spam this issue too much?). |
@arunpersaud , sure you can reach out (just check my profile to see how you prefer to do so)! However, bear in mind that my time for coaching is somewhat limited at the moment. You can also check this meta-repository, which has tons of interesting people involved with python+instrumentation, and this catalog of projects |
WOW @aquilesC what amazingly detailed review---just amazing. I'd like to give a few humble "big picture" comments as someone who is interested in this kind of software and is also working in this space. First off, I agree that ideally you should find an existing framework and contribute to that rather than inventing your own approach for driver abstraction. We have a bunch of competing approaches right now, and what we need most of all is to grow an actually sustainable community around something. The dream, of course, is to have a community that works together within one project to do a whole bunch of hardware enablement that everyone can benefit from. micro-manager and EPICS are examples of communities that have succeeded in developing this critical mass, although in my opinion each of them fails to meet the needs of small, general-purpose labs like we're interested in here. I'm generally not a fan of monolithic packages that attempt to implement all of their hardware support and graphics together---this leads to packages that are hard to install and not very portable or extensible in my opinion. I myself am currently ripping out hardware support from pycmds and migrating everything to yaq. At the very least, you should comment on the projects mentioned by @aquilesC and contrast them with hardware-control within the paper. On the other hand, I actually think that your front-end approach is compelling. The only other package that I know of is PyDM, but other than that I'm not aware of others who are attempting to solve the frontend problem generically. I encourage you to look closer at the existing ecosystem and think carefully about where you can best contribute without reinventing. Perhaps for example you could focus on leveraging an existing driver framework and creating a really polished GUI experience for particle physics. |
Regarding publication, I think I'm less pessimistic than @aquilesC. My understanding is that JOSS does not review based on novelty. I think that hardware-control could be reasonable JOSS publication if the authors meet the following criteria:
I personally think that things like type hints and exception handling don't need to be perfect for publication. To me it's clear that, while this package has room to grow for sure, it definitely passes JOSS' core requirements:
Perhaps the editor could provide some guidance here? @timtroendle |
@aquilesC can you say more here? I'm not sure I understand your point. I understand that daqmx isn't message based, but I do think I could write a class that parameterizes the functionality of an NI card into hardware-control messages. |
@untzag thanks for the review. Both reviews are very helpful and we will change our project to incorporate them. Perhaps merging the backends into an existing project would be the way to go (which would make our codebase a lot smaller). Also adding tests seems more doable now after I have seen how other projects are handling this. This will take us a while though (including reviewing all the other projects that both of you pointed out). Should we close this and resubmit later or keep this open and let you guys know when we have a newer version ready? Perhaps another question for the editor ;) @timtroendle |
@untzag , perhaps I should have made it clearer. I do agree with you, and I followed the checklist. I checked the 'scholarly effort' that mentions the JOSS guidelines. However, there are other things that I could not tick, and I included this sentence in my original review:
Then, the rest of the suggestions are to be taken as that, an opportunity to improve the code and not the mininum effort that needs to be put to get the publication ready. The authors are free to choose the path they desire. |
Sorry, the economy of words made the sentence miss the point. In the package they can use an NI card because there is a python wrapper for the underlying C library. The rest of the backends work with message-based devices (at least, the ones I remember checking). If I wanted to include another type of device, such as a camera, or a digitizer that streams data, there is no clear path within the documentation, and the approach they followed in the other backends would not be easy to replicate. I think it is fair to focus on message-based if those are the ones you use the most, but then it should have been nice to mention it because it makes their use-case more explicit. |
Thanks for these amazing reviews, @aquilesC and @untzag . Regarding the two questions raised: First, yes, not all points mentioned are necessary for a publication in JOSS. If there were any specific points for which you are unsure whether they are necessary, @arunpersaud , I'd suggest to bring them up here so we can discuss. However, you already indicated that you are interested in implementing the requested changes and that brings me to the second question: If you need time to implement the changes, @arunpersaud , I suggest to pause this review and to continue whenever you are ready, instead of closing and resubmitting. Would this be a problem for any of the reviewers @aquilesC, @untzag , @garrettj403 ? @arunpersaud , please give us a rough estimation of how long this'll take you. |
I'm happy to review later if that's what @arunpersaud wants to do. |
Fine with me! |
Let's pause it then and we'll come back here once we updated the code. It's hard for me to estimate how long this will take though. Lot's to review and change and unfortunately working on the code is not the main part of my job. I would say we need a few month at least. |
👋 @arunpersaud – just checking in here. Do you think you might be able to complete your changes in the next month? |
👋 @openjournals/joss-eics, this paper is ready to be accepted and published. Check final proof 👉 openjournals/joss-papers#3129 If the paper PDF and the deposit XML files look good in openjournals/joss-papers#3129, then you can now move forward with accepting the submission by compiling again with the command |
also want to thank @untzag, @garrettj403, @aquilesC, @arfon, and @timtroendle for their time and help! |
Congrats @arunpersaud, really great work. |
👋 @arunpersaud – I just made a number of minor updates to your paper here: https://bitbucket.org/berkeleylab/hardware-control/pull-requests/1 |
Thanks, just merged them... |
@editorialbot recommend-accept |
|
@arunpersaud – apologies but it looks like this line got broken by my commit. Could you change L33 back to:
|
@arfon just pushed a fix from my laptop (but couldn't run the docker container to check if it worked). Let me know if this didn't fix it and I will work on it later and test before commiting |
@editorialbot recommend-accept |
|
|
👋 @openjournals/joss-eics, this paper is ready to be accepted and published. Check final proof 👉 openjournals/joss-papers#3148 If the paper PDF and the deposit XML files look good in openjournals/joss-papers#3148, then you can now move forward with accepting the submission by compiling again with the command |
@editorialbot accept |
|
🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦 |
🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨 Here's what you must now do:
Any issues? Notify your editorial technical team... |
@aquilesC, @untzag, @garrettj403 – many thanks for your reviews here and to @timtroendle for editing this submission! JOSS relies upon the volunteer effort of people like you and we simply wouldn't be able to do this without you ✨ @arunpersaud @Grant-Giesbrecht – your paper is now accepted and published in JOSS ⚡🚀💥 |
🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉 If you would like to include a link to your paper from your README use the following code snippets:
This is how it will look in your documentation: We need your help! The 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:
|
Submitting author: @Grant-Giesbrecht (Grant Giesbrecht)
Repository: https://bitbucket.org/berkeleylab/hardware-control/src/main/
Branch with paper.md (empty if default branch):
Version: 2.1.0
Editor: @timtroendle
Reviewers: @aquilesC, @untzag, @garrettj403
Archive: 10.5281/zenodo.6459291
Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.
Status
Status badge code:
Reviewers and authors:
Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)
Reviewer instructions & questions
@aquilesC & @untzag & @garrettj403, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @timtroendle know.
✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨
Review checklist for @aquilesC
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @untzag
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @garrettj403
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
The text was updated successfully, but these errors were encountered: