search

openjournals / joss-reviews

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

[REVIEW]: pirecorder: Controlled and automated image and video recording with the raspberry pi #2584

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @JolleJolles (Jolle Wolter Jolles) Repository: http://github.com/jollejolles/pirecorder Version: v3.2.0 Editor: @marcosvital Reviewer: @lucask07, @DerJH Archive: 10.5281/zenodo.4058628

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

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

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 & @DerJH, 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 @marcosvital 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 @lucask07

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @DerJH

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

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

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

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

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1038/s41598-019-43845-9 is OK
- 10.1111/2041-210X.13005 is OK
- 10.1371/journal.pcbi.1006959 is OK
- 10.1371/journal.pbio.2003026 is OK
- 10.1016/j.cub.2017.08.004 is OK
- 10.1098/rspb.2017.2629 is OK
- 10.1016/j.anbehav.2019.06.022 is OK
- 10.1371/journal.pone.0169758 is OK
- 10.1016/j.dsr.2019.103136 is OK
- 10.1111/jofo.12182 is OK
- 10.1371/journal.pone.0220751 is OK
- 10.1089/zeb.2016.1412 is OK
- 10.3389/fevo.2018.00026 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 4 years ago

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

marcosvital commented 4 years ago

Dear @JolleJolles: your manuscript will be reviewed in this issue, and you can reply any comments and suggestions that the reviewers might address right here.

marcosvital commented 4 years ago

@lucask07 and @DerJH: thank you all for for accepting review this submission for JOSS.

Even if you are not starting the review right now, please accept the invite, as it has an expiration date (there is a link under Reviewer instructions & questions and you should also get an email notification). Furthermore, please check the instructions and checklists above, and let me know if you need any assistance.

You can also tag @JolleJolles if you need to ask specific questions about the submission.

JolleJolles commented 4 years ago

Thanks everyone for your help. Looking forward.

On 20 Aug 2020, at 12:46, Marcos Vital (LEQ-UFAL) notifications@github.com wrote:

Dear @JolleJolles https://github.com/JolleJolles: your manuscript will be reviewed in this issue, and you can reply any comments and suggestions that the reviewers might address right here.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/2584#issuecomment-677521845, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACC6PYPS6YTK3MUYMAYXTDTSBT5HNANCNFSM4QF5CM5A.

DerJH commented 4 years ago

Hi, I am trying to install pirecorder at the moment, but both using sudo pip install pirecorder and the GitHub version i am running into the following error code (I attached the whole commandline dialog as a text file). It seems to have something to do with "h5py". I then tried to install with "sudo pip install h5py", which also failed.

Command "/usr/bin/python -u -c "import setuptools, tokenize;file='/tmp/pip-install-eCsuDf/h5py/setup.py';f=getattr(tokenize, 'open', open)(file);code=f.read().replace('\r\n', '\n');f.close();exec(compile(code, file, 'exec'))" install --record /tmp/pip-record-Vphw1e/install-record.txt --single-version-externally-managed --compile" failed with error code 1 in /tmp/pip-install-eCsuDf/h5py/

Is there an obvious workaround or fix for that? Best, Jan full_error.txt

JolleJolles commented 4 years ago

Hi Jan,

That seems to be an error with the h5py package, not with pirecorder. I have never encountered it. I have installed pirecorder on fresh raspbian systems on rpi2,3, and 4 and then do not get this error. I am not sure how to solve the h5 install error, perhaps try updating your raspberry pi?

h5py is not actually used by pirecorer but only by 1 function in my pythutils package that is also not used by pirecorder. I updated pythutils to not require the h5py package anymore, so hopefully now when installing pythutils and pirecorder you can get past this stage. You have to use the github version (1.3.6). I will update it on pypi.org after the package and paper are passed the review stage.

I hope this helps, do let me know if something else related to this point has come up.

Thanks, Jolle

DerJH commented 4 years ago

Hi Jolle, ok after upgrading raspbian and using apt-get python-h5py and apt-get python3-h5py the installation worked! I'll try to test all the functions in your program in the next 2 days. Greetings, Jan

lucask07 commented 4 years ago

Hi @marcosvital, Sorry, I just came to this and the invite has expired. Is there a way you could resend?

lucask07 commented 4 years ago

Hi @JolleJolles, A quick question before I dig too deep. Is your package targeting a specific Raspberry Pi compatible camera? Have you documented the cameras your software has been tested with? I suspect the compatible cameras are set by the Picamera package. I see these "official" Raspberry Pi cameras that connect via the MIPI port here

JolleJolles commented 4 years ago

Hi @lucask07. No the package does not target a specific camera and should work with all cameras, official and non-official, that connect with the MIPI port, as pirecorder builds on the picamera package. Thanks

marcosvital commented 4 years ago

@whedon re-invite @lucask07 as reviewer

whedon commented 4 years ago

OK, the reviewer has been re-invited.

@lucask07 please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

marcosvital commented 4 years ago

Let me know if it's working, @lucask07.

lucask07 commented 4 years ago

Hi @marcosvital It's working, I can click on the check-boxes now.

lucask07 commented 4 years ago

@JolleJolles

A few comments as I'm working through the Documentation section of the reviewer checklist.

lucask07 commented 4 years ago

For the scholarly contribution a check with CLOC ("count lines of code") shows:

So this work is above the LOC threshold. Submissions with LOC less than 1000 are "flagged" per the JOSS scholarly effort guidelines.

JolleJolles commented 4 years ago

@JolleJolles

A few comments as I'm working through the Documentation section of the reviewer checklist.

  • README page is missing an explanation of how others may "1) Contribute to the software"
  • The Statement of Need should be elaborated on in both the README and the github.io page.
  • The software does not have automated tests. "Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?"

Thanks @lucask07. I will address your comments 1 and 2 immideately.

For Comment 3, I read in the author guidelines that automated tests are not a requirement and that "Documented manual steps that can be followed to objectively check the expected functionality of the software" would also be okay. I have detailed tutorials to run through the whole software (jollejolles.github.io/pirecorder), which I thought, perhaps wrongly so, that that would conform to that requirement. Would that indeed be enough if I would add a clear statement that the software can be tested by following the online manual? Or alternatively if I add python files that can be run to show the software is working? I would appreciate your feedback on this so I can address the comment appropriately.

JolleJolles commented 4 years ago

For the scholarly contribution a check with CLOC ("count lines of code") shows:

  • Python: 1093 lines of code, and 402 lines of comment
  • Markdown 1150 lines of code
  • Jupyter Notebook: 52 lines of code, 973 lines of comment

So this work is above the LOC threshold. Submissions with LOC less than 1000 are "flagged" per the JOSS scholarly effort guidelines.

Thanks. I have actually very limited comments throughout my code. The only commented code would be the function documentation. Could you guide me on what to here? Thank you.

DerJH commented 4 years ago

@JolleJolles A few comments as I'm working through the Documentation section of the reviewer checklist.

  • README page is missing an explanation of how others may "1) Contribute to the software"
  • The Statement of Need should be elaborated on in both the README and the github.io page.
  • The software does not have automated tests. "Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?"

Thanks @lucask07. I will address your comments 1 and 2 immideately.

For Comment 3, I read in the author guidelines that automated tests are not a requirement and that "Documented manual steps that can be followed to objectively check the expected functionality of the software" would also be okay. I have detailed tutorials to run through the whole software (jollejolles.github.io/pirecorder), which I thought, perhaps wrongly so, that that would conform to that requirement. Would that indeed be enough if I would add a clear statement that the software can be tested by following the online manual? Or alternatively if I add python files that can be run to show the software is working? I would appreciate your feedback on this so I can address the comment appropriately.

Hi, I think a python file which executes several functions would be a good idea, but as I am also new to the software testing business (and github reviewing) not sure if that is the way these things go about.

DerJH commented 4 years ago

I have some more questions/comments

  1. Is there a way to save the ROI directly into the config file? So 1. opening stream, 2. making rectangle, 3. press "s", 4. save to the current .conf file?

  2. In the convert function I might have discovered a tiny bug.

from pirecorder import Convert Convert(indir = "/home/pi/Videos", outdir = "/home/pi/Videos/converted", resizeval = 0.5) Convert(indir = "/home/pi/Videos", outdir = "/home/pi/Videos/converted", resizeval = 0.5, withframe = True)

when i do the "withframe = True" then the result becomes a scrambled video mostly grey and random pix, but it works without.

  1. The following example on the website has a small error. Twice "imgwait": rec.settings(imgwait = 1, imgnr = 10, imgtime = 15, imgwait = 60, imgquality = 90 \ imgdims = (3280, 2464))

4.This is more a wish for a future update: On a small screen the pirecorder.Camconfig() slider window only allows access until the first Red channel slider. Maybe this could be made more responsive, or all the UI elements a bit smaller.

  1. all the above is tested using python 2.7. I somehow didn't manage to get it to work under python 3, but given my general linux skills this is probably more a problem of me failing to manage to install it properly. But maybe you can help me to figure out what could be wrong here. pi@raspberrypi:~ $ python3 Python 3.7.3 (default, Jul 25 2020, 13:03:44) [GCC 8.3.0] on linux Type "help", "copyright", "credits" or "license" for more information.

    import cv2 import pirecorder rec = pirecorder.PiRecorder(configfile = "irsettings.conf") Traceback (most recent call last): File "", line 1, in AttributeError: module 'pirecorder' has no attribute 'PiRecorder' exit()

thanks

lucask07 commented 4 years ago

For the scholarly contribution a check with CLOC ("count lines of code") shows:

  • Python: 1093 lines of code, and 402 lines of comment
  • Markdown 1150 lines of code
  • Jupyter Notebook: 52 lines of code, 973 lines of comment

So this work is above the LOC threshold. Submissions with LOC less than 1000 are "flagged" per the JOSS scholarly effort guidelines.

Thanks. I have actually very limited comments throughout my code. The only commented code would be the function documentation. Could you guide me on what to here? Thank you.

@JolleJolles : I think this is fine and there's no needed action on your part. I was just documenting my findings.

JolleJolles commented 4 years ago

@lucask07 it would be great to get your response to my reply earlier this week so I can best address your points. Thanks

@JolleJolles A few comments as I'm working through the Documentation section of the reviewer checklist.

  • README page is missing an explanation of how others may "1) Contribute to the software"
  • The Statement of Need should be elaborated on in both the README and the github.io page.
  • The software does not have automated tests. "Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?"

Thanks @lucask07. I will address your comments 1 and 2 immideately.

For Comment 3, I read in the author guidelines that automated tests are not a requirement and that "Documented manual steps that can be followed to objectively check the expected functionality of the software" would also be okay. I have detailed tutorials to run through the whole software (jollejolles.github.io/pirecorder), which I thought, perhaps wrongly so, that that would conform to that requirement. Would that indeed be enough if I would add a clear statement that the software can be tested by following the online manual? Or alternatively if I add python files that can be run to show the software is working? I would appreciate your feedback on this so I can address the comment appropriately.

JolleJolles commented 4 years ago

With regards to revising the paper/package, would it be best to wait for all comments to incorporate them all at once in a new release or address them as they come in? I have no experience in this submission and revision process via Github yet so appreciate your understanding. Thanks!

marcosvital commented 4 years ago

With regards to revising the paper/package, would it be best to wait for all comments to incorporate them all at once in a new release or address them as they come in? I have no experience in this submission and revision process via Github yet so appreciate your understanding. Thanks!

It's pretty much up to you, @JolleJolles. Most authors will work on improvements and corrections as they come, but it's not a rule at all. So feel free to follow the path you think is better.

Now, if you choose to go working as the suggestions come in, I would suggest you to address the changes you make here in this issue, so reviewers can easily track down what is already covered. Also, both you and the reviewers can open issues on the software's repository to track down important issues/changes and link them here.

DerJH commented 4 years ago

Hi @JolleJolles Just a small comment about the "statement of need". I think it would be sufficient to basically just place a header after the first summary paragraph, as you clearly outline why you made the software there.

You could also place a similar statement of need paragraph on the front page of the software documentation and then I guess the two JOSS points about the "statement of need" would be addressed.

lucask07 commented 4 years ago

Hi @JolleJolles, I'm going through the paper and have a few concerns/nitpicks that I'd suggest you clarify.

Thanks for all your work.

JolleJolles commented 4 years ago

Thanks for all your comments. I will address them all together from next Monday (14th), making sure to add each point as an issue to the repository. Thanks

JolleJolles commented 4 years ago

4.This is more a wish for a future update: On a small screen the pirecorder.Camconfig() slider window only allows access until the first Red channel slider. Maybe this could be made more responsive, or all the UI elements a bit smaller.

I opened this issue as an enhancement request (https://github.com/JolleJolles/pirecorder/issues/28) and addressed it there with this commit.

JolleJolles commented 4 years ago
  1. The following example on the website has a small error. Twice "imgwait": rec.settings(imgwait = 1, imgnr = 10, imgtime = 15, imgwait = 60, imgquality = 90 imgdims = (3280, 2464))

Thanks, fixed the typo with commit c2f23e4

JolleJolles commented 4 years ago

A few comments as I'm working through the Documentation section of the reviewer checklist.

  • README page is missing an explanation of how others may "1) Contribute to the software"
  • The Statement of Need should be elaborated on in both the README and the github.io page.
  • The software does not have automated tests. "Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?"

Following the above point, I improved the statement of need in the paper and added the statement to the README and the web documentation (issue #23, commit edae82f).

JolleJolles commented 4 years ago
  • The code camconfig.py allows for a limited range of ISO.
isos = [100,200,320,400,500,640,800]

Is this consistent with the capabilities of the 3 camera modules? Is it possible to probe the camera module for the available ISO settings?

In reply to this point, pirecorder builts on the picamera package to control the native camera settings, which are indeed limited to the above range, see the pirecorder documentation. pirecorder uses a default iso setting of 200.

JolleJolles commented 4 years ago

All the above is tested using python 2.7. I somehow didn't manage to get it to work under python 3, but given my general linux skills this is probably more a problem of me failing to manage to install it properly. But maybe you can help me to figure out what could be wrong here. pi@raspberrypi:~ $ python3 Python 3.7.3 (default, Jul 25 2020, 13:03:44) [GCC 8.3.0] on linux Type "help", "copyright", "credits" or "license" for more information.

import cv2 import pirecorder rec = pirecorder.PiRecorder(configfile = "irsettings.conf") Traceback (most recent call last): File "", line 1, in AttributeError: module 'pirecorder' has no attribute 'PiRecorder' exit()

@DerJH in reply to your above question, I cannot reproduce your issue. Doing a fresh install of pirecorder on python3 I do not get this error. Could you show the output of pirecorder.__version__ and cv2.__version so I can help further? I also recommend to use a virtual environment (as I detail in the online documentation)

JolleJolles commented 4 years ago
  1. Is there a way to save the ROI directly into the config file? So 1. opening stream, 2. making rectangle, 3. press "s", 4. save to the current .conf file?

Integrated saving directly from stream by the possibility of adding a configuration file when running stream from the command line (e.g. stream -c pirecorder.conf). When the 's' key is pressed after drawing a rectangle, the roi coordinates are stored in the config file. Working after commit d335de9.

JolleJolles commented 4 years ago

2. In the convert function I might have discovered a tiny bug.

from pirecorder import Convert Convert(indir = "/home/pi/Videos", outdir = "/home/pi/Videos/converted", resizeval = 0.5) Convert(indir = "/home/pi/Videos", outdir = "/home/pi/Videos/converted", resizeval = 0.5, withframe = True)

when i do the "withframe = True" then the result becomes a scrambled video mostly grey and random pix, but it works without.

@DerJH I also cannot reproduce this issue. When the withframe parameter is set to True then instead of fast ffmpeg conversion, opencv will be used for conversion (and drawing the frame number on each frame of the video). So I think there is an issue with the version of opencv you are using. Python2.7 is no longer supported and I think it could well be that your version of opencv is not up to par anymore. Again I recommend to try again on python3 and to use a virtual environment to install the right version of opencv. I wrote a special documentation page for that, but it should hopefully work after running this simple command: pip3 install opencv-contrib-python==4.1.0.25.

JolleJolles commented 4 years ago

I have some more questions/comments

  1. Is there a way to save the ROI directly into the config file? So 1. opening stream, 2. making rectangle, 3. press "s", 4. save to the current .conf file?
  2. In the convert function I might have discovered a tiny bug.

from pirecorder import Convert Convert(indir = "/home/pi/Videos", outdir = "/home/pi/Videos/converted", resizeval = 0.5) Convert(indir = "/home/pi/Videos", outdir = "/home/pi/Videos/converted", resizeval = 0.5, withframe = True)

when i do the "withframe = True" then the result becomes a scrambled video mostly grey and random pix, but it works without.

  1. The following example on the website has a small error. Twice "imgwait": rec.settings(imgwait = 1, imgnr = 10, imgtime = 15, imgwait = 60, imgquality = 90 imgdims = (3280, 2464))

4.This is more a wish for a future update: On a small screen the pirecorder.Camconfig() slider window only allows access until the first Red channel slider. Maybe this could be made more responsive, or all the UI elements a bit smaller.

  1. all the above is tested using python 2.7. I somehow didn't manage to get it to work under python 3, but given my general linux skills this is probably more a problem of me failing to manage to install it properly. But maybe you can help me to figure out what could be wrong here. pi@raspberrypi:~ $ python3 Python 3.7.3 (default, Jul 25 2020, 13:03:44) [GCC 8.3.0] on linux Type "help", "copyright", "credits" or "license" for more information.

import cv2 import pirecorder rec = pirecorder.PiRecorder(configfile = "irsettings.conf") Traceback (most recent call last): File "", line 1, in AttributeError: module 'pirecorder' has no attribute 'PiRecorder' exit()

thanks

I hope to have hereby addressed all your comments from this thread. Please follow-up on the specific comments above if needed.

JolleJolles commented 4 years ago

A few comments as I'm working through the Documentation section of the reviewer checklist.

  • README page is missing an explanation of how others may "1) Contribute to the software"
  • The Statement of Need should be elaborated on in both the README and the github.io page.
  • The software does not have automated tests. "Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?"

I have now included a python test file in the package (tests/test.py) to run all functionalities, including running pirecorder, setting configuration files, running different types of recordings, scheduling recordings, converting recordings, and testing the stream and camconfig interactive functions. The possibility to test the software is also described in the README and in the online documentation.

DerJH commented 4 years ago
  1. In the convert function I might have discovered a tiny bug. from pirecorder import Convert Convert(indir = "/home/pi/Videos", outdir = "/home/pi/Videos/converted", resizeval = 0.5) Convert(indir = "/home/pi/Videos", outdir = "/home/pi/Videos/converted", resizeval = 0.5, withframe = True) when i do the "withframe = True" then the result becomes a scrambled video mostly grey and random pix, but it works without.

@DerJH I also cannot reproduce this issue. When the withframe parameter is set to True then instead of fast ffmpeg conversion, opencv will be used for conversion (and drawing the frame number on each frame of the video). So I think there is an issue with the version of opencv you are using. Python2.7 is no longer supported and I think it could well be that your version of opencv is not up to par anymore. Again I recommend to try again on python3 and to use a virtual environment to install the right version of opencv. I wrote a special documentation page for that, but it should hopefully work after running this simple command: pip3 install opencv-contrib-python==4.1.0.25.

I am also convinced that this is some installation hiccup with python and opencv on my side. I will try to check this on a fresh install in the coming days. But given that it worked for @lucask07 it should not hinder the publication.

lucask07 commented 4 years ago
  1. In the convert function I might have discovered a tiny bug. from pirecorder import Convert Convert(indir = "/home/pi/Videos", outdir = "/home/pi/Videos/converted", resizeval = 0.5) Convert(indir = "/home/pi/Videos", outdir = "/home/pi/Videos/converted", resizeval = 0.5, withframe = True) when i do the "withframe = True" then the result becomes a scrambled video mostly grey and random pix, but it works without.

@DerJH I also cannot reproduce this issue. When the withframe parameter is set to True then instead of fast ffmpeg conversion, opencv will be used for conversion (and drawing the frame number on each frame of the video). So I think there is an issue with the version of opencv you are using. Python2.7 is no longer supported and I think it could well be that your version of opencv is not up to par anymore. Again I recommend to try again on python3 and to use a virtual environment to install the right version of opencv. I wrote a special documentation page for that, but it should hopefully work after running this simple command: pip3 install opencv-contrib-python==4.1.0.25.

I am also convinced that this is some installation hiccup with python and opencv on my side. I will try to check this on a fresh install in the coming days. But given that it worked for @lucask07 it should not hinder the publication.

Hi @DerJH, unfortunately, I do not have a Raspberry Pi and so am not able to test for functionality.

lucask07 commented 4 years ago

@JolleJolles Looks great that most issues are closed. I'm OK with signing off on publication once the wording on camera calibration is cleared up in the paper. Note that I will need to leave the functionality verification box unchecked since I lack the hardware.

JolleJolles commented 4 years ago

Hi @JolleJolles, I'm going through the paper and have a few concerns/nitpicks that I'd suggest you clarify.

  • The paper references "get the optimal camera shutter speed". This is a rather complex calculation that attempts to limit saturation while simultaneously having the darkest pixels collect the most possible light. I can't find any auto exposure calculations in your code. Can you clarify how the optimal shutter speed is calculated/retrieved?

In this context, it was meant optimal in terms of the user's perspective. I rewrote the sentence to say "...set the ideal camera shutter speed" to make this clearer.

  • The paper reference “Calibrating the camera.” This is not appropriate usage of the term calibrate for a camera. Camera calibration may involve determining the focal length and image center or estimating the lens distortion. Other calibration efforts may center around aspects of sensor performance such as measuring the color correction matrix or locating hot pixels. This does not appear to be what pirecorder is doing. I think your package enables real-time streaming so that a user is given feedback while positioning a camera? Can you change the paper wording to clarify what pirecorder helps the user accomplish?

One of the definitions of "calibrate" is to "carefully assess, set, or adjust". This is what is meant here. By means of an interactive user stream, both with the stream and camconfig functions, the user is for example able to carefully position the camera and center it, adjust the camera focus using a zoomed-in view of the camera, and adjust external lighting and camera sensitivity settings to get the ideal image brightness and shutter speed. I, therefore, do feel that the term "calibrate" is correctly used here. But I agree that this was not clear enough in the paper. I have therefore carefully revised the paper to better clarify what pirecorder helps accomplish in terms of the steps until recording.

  • Paper grammar: “Multiple configuration files can be used that can be edited directly, such as to set the video duration, quality, or number of images, or dynamically via an interactive video stream, such as to draw the region of interest, set the desired white-balance, or get the optimal camera shutter speed.” The “or”s in this list are confusing. Should these actually be “and”s? Consider splitting this sentence into 2 or 3 sentences for clarity.

I have revised this paragraph to improve grammar and readability, including by splitting the sentence into multiple sentences.

  1. v1.5 camera model (2592x1944)
  2. v2 camera (3280x2464)
  3. HQ camera (4056x3040) I think these 3 cameras should be more clearly called out in the README.

These are the three official cameras, all with their own maximum resolution. But besides these three official cameras there are many unofficial cameras that also connect to the MIPI port and should generally be supported. I have revised the sentences related to this point in the Jupiter notebook to clarify to the user that the maximum resolution depends on the camera used and that "for example" the three official cameras have max resolutions of (2592,1944), (3280x2464) and (4056x3040). I have also renamed the cameratype parameter to maxres with the ability to set the resolution via a tuple as well as a string detailing either "v1.5", "v2", or "hq", to help users get the maximum out of their camera.

  • The code camconfig.py allows for a limited range of ISO.
isos = [100,200,320,400,500,640,800]

Is this consistent with the capabilities of the 3 camera modules? Is it possible to probe the camera module for the available ISO settings?

See my reply in a previous comment.

  • The code camconfig.py appears to limit resolution to 1640x1232:
if (res[0]*res[1])>=(1640*1232):
        res = maxrect(dims, maxdims = (1640,1232), decimals = 0)
cam.resolution = picamconv(res)

Is my interpretation correct? Is this limitation required?

Yes, this limitation was included to prevent users from using a too high resolution for streaming and getting a shuttering video. The limitation could be removed but most users want to use a smaller video with good frame rate anyway to calibrate the camera settings as that is what the camconfig function is about.

JolleJolles commented 4 years ago

@JolleJolles Looks great that most issues are closed. I'm OK with signing off on publication once the wording on camera calibration is cleared up in the paper. Note that I will need to leave the functionality verification box unchecked since I lack the hardware.

I have revised the paper in a number of ways, including removing the word "calibration" and adding a couple of sentences that clarify the streaming functionalities among some other changes.

JolleJolles commented 4 years ago

I hope to have hereby addressed all your comments! Many thanks for the very helpful feedback, I really appreciate you taking the time. And please let me know if there are any final things you'd like me to fix or clarify.

DerJH commented 4 years ago

I am also happy with the changes and the current state of the submission. And as I could confirm the functionality on python 2.7 I can also tick the functionality part. I really enjoyed playing around with the software, and I hope to apply it in my coming experiments myself.

JolleJolles commented 4 years ago

Great! I acknowledged both your help in the paper. I have created an explanatory video for pirecorder that I want to release when the paper will be available online. I will integrate a final slide with the details of the paper, but you can find a temporary version here: https://youtu.be/aEWIGlDY3f8

lucask07 commented 4 years ago

Thanks @JolleJolles. @marcosvital I approve this submission.

JolleJolles commented 4 years ago

Dear @marcosvital, please let me know if there is anything else you'd like me to do. Many thanks.

marcosvital commented 4 years ago

Hi, @JolleJolles, I think we are ready to go through our final steps before publication. I'll start with a simple reference's DOI check.

@lucask07 and @DerJH, thank you so much for all the time and effort you put into reviewing this submission for JOSS.

marcosvital commented 4 years ago

@whedon check references