search

openjournals / joss-reviews

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

[REVIEW]: Optimising Light Source Positioning for Even and Flux-Efficient Illumination #1392

Closed whedon closed 5 years ago

whedon commented 5 years ago

Submitting author: @adrena-lab (Stefan Guldin) Repository: https://github.com/adrena-lab/Optimising-Light-Source-Positioning Version: 1.0.1 Editor: @trallard Reviewer: @cmccomb, @nicoguaro Archive: 10.5281/zenodo.2847905

Status

status

Status badge code:

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

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

@cmccomb & @nicoguaro, 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.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @trallard know.

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

Review checklist for @cmccomb

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @nicoguaro

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 5 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @cmccomb, 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:

  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 commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

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

/app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:in block in find': No such file or directory - tmp/1392 (Errno::ENOENT) from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:incollect!' from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:in find' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-a1723d160bb6/lib/whedon/processor.rb:57:infind_paper_paths' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-a1723d160bb6/bin/whedon:50:in prepare' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/command.rb:27:inrun' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:in invoke_command' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor.rb:387:indispatch' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/base.rb:466:in start' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-a1723d160bb6/bin/whedon:116:in<top (required)>' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in load' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in

'

trallard commented 5 years ago

@nicoguaro @cmccomb thanks for agreeing to review this submission.

You will find a checklist for each of you, please tick the items as you go over them. Use this issue to keep the updates and discussions in a central place. However, if there are longer issues or improvements that need to be requested, please make an issue on the software repo and reference it here 😄

If you encounter any issues or have any questions about the review process feel free to ping me here

cmccomb commented 5 years ago

@adrena-lab This is a great repository! Here's what I think should be updated/revised:

  1. General Checks, Version: You should make a release with version 1.0.
  2. Documentation, Installation instructions: Please state the dependencies. For MATLAB, this probably just means to indicate which versions it should run on. If you want to make claims for Octave (recommended!) please provide version information for that too. It would be helpful to provide a short description fo what each file does, as well as a description of what the .mat file is used for.
  3. Documentation, Automated Tests: There is not way to check the functionality of the software. Maybe the authors could provide an expected optimum value for the example that users could compare against?
  4. Documentation, Example Usage: Will the authors please provide some more description of the example case in the README file? This would be helpful for the reader. The content can be borrowed from the software paper.
  5. Documentation, Community Guidelines: Community guidelines should be added. In addition, the zip file should not be used in the repository in order to make it easier for others to contribute. Include the .m and .mat files directly.
nicoguaro commented 5 years ago

@adrena-lab, I downloaded the repository and ran the main file using Octave 5.1 under Windows 10. I agree with the comments added by @cmccomb.

I will test the software in MATLAB later, in the mean time I add the errors I obtained using Octave.

The first error was 

error: 'fullfact' undefined near line 44 column 16
error: called from
    Create_Variable_Combinations at line 44 column 14
    Optimising_Light_Source_Positioning at line 62 column 16

This was fixed doing the following in the command prompt.

pkg load statistics

Similarly, I obtained the following error

error: 'std2' undefined near line 171 column 28
error: called from
    Illumination_Calculations at line 171 column 26
    Optimising_Light_Source_Positioning at line 158 column 21

That was fixed with

pkg load image

There is also the following error:

error: 'newline' undefined near line 176 column 13
error: called from
    Convhull_Option_Reduction at line 176 column 5
    Optimising_Light_Source_Positioning at line 189 column 29
StefanGuldin commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

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

/app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:in block in find': No such file or directory - tmp/1392 (Errno::ENOENT) from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:incollect!' from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:in find' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-a1723d160bb6/lib/whedon/processor.rb:57:infind_paper_paths' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-a1723d160bb6/bin/whedon:50:in prepare' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/command.rb:27:inrun' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:in invoke_command' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor.rb:387:indispatch' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/base.rb:466:in start' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-a1723d160bb6/bin/whedon:116:in<top (required)>' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in load' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in

'

StefanGuldin commented 5 years ago

Dear @trallard, @cmccomb and @nicoguaro

thank you for handling the manuscript and for the very constructive feedback to our work. We have in the meantime uploaded a revised submission on github. We restructured the organisation of the files in the repository and made major changes to the file readme.md:

1) We expanded the installation instructions to include a description of what each file does. 2) We have added a section on dependencies. 3) We have added/expanded the Example Usage Section to include a description of the example case and how it is carried out. Further, we note that the software is automatically set up to run this example to act as an automated test. Anyone who runs the code as provided will get the same results as shown in the example. 4) We have also added some notes on the community guidelines. 5) We have made a release version v1.0 to package the software

Please note that 1) We received an error message from whedon about the pdf file compilation. Whedon was able to compile the pdf previously and since then, we did not change anything to the relevant file paper.md. Would you be able to resolve this issue? 2) Please note that we have no previous experience in the use of Octave. Based on the comments of @nicoguaro, it seems to be relatively straight forward to run our files, but we feel unable to facilitate this and would prefer to leave the adaptation to other platforms to the user.

We hope to have fully addressed all points raised by the reviewers and look forward to your response. Thank you and best wishes, @adrena-lab

cmccomb commented 5 years ago

@adrena-lab Thank you for the detailed response and revisions! I have updated my review checklist and have no further recommendations.

nicoguaro commented 5 years ago

I tested the software using MATLAB 2019a and obtained the results presented in the example. Nevertheless, I obtained the following warning message:

Warning: Polynomial is badly conditioned. Add points with distinct X values, reduce the degree of the polynomial, or try centering
and scaling as described in HELP POLYFIT. 
> In polyfit (line 79)
  In Convhull_Option_Reduction (line 91)
  In Optimising_Light_Source_Positioning (line 188) 
Warning: Polynomial is badly conditioned. Add points with distinct X values, reduce the degree of the polynomial, or try centering
and scaling as described in HELP POLYFIT. 
> In polyfit (line 79)
  In Convhull_Option_Reduction (line 91)
  In Optimising_Light_Source_Positioning (line 188)

Comments

StefanGuldin commented 5 years ago

Dear @trallard, @cmccomb and @nicoguaro thanks for the feedback we received on our recent revision, in particular the comments by @nicoguaro. We have now further revised our contribution as follows: • We fixed a Minor bug in Convhull_Option_Reduction.m to remove warning message • We fixed a minor bug in Plot_Save_Results.m to ensure the rectangle denoting the illuminated area and circles that represent the positions of the light sources are fully visible and not obstructed by the surface plot. • We added a statement of need section to README.md for clarity • We reformatted README.md file to separate installation instructions from the description of each .m & .mat file • We added help block to each function file with information defining the input and output parameters • We edited the code to ensure all lines of code and comments are not too long • Created new release (v1.0.1) to cover addition of help blocks and aforementioned bug fixes Our repository on Github is updated accordingly. Again, we are extremely grateful for the time spent on reviewing and testing our JOSS submission and for the constructive feedback that we received during the revision process. We hope that the contribution is now suitable for publication in JOSS. Best wishes, Niamh, Alaric and Stefan from @adrenlab

nicoguaro commented 5 years ago

I would recommend the paper for publication now.

trallard commented 5 years ago

Hey @adrena-lab it seems all is now good for publication, can you please go over this list of actions before I move onto the process:

  1. Create a new release on GitHub
  2. Publish the package in Zenodo and share the DOI here (the general one pointing to the code and all the versions, not the one corresponding to the latest release)
  3. Check the paper for: links, authors and DOIs and call @whedon generate pdf

Once this is completed please ping me directly

StefanGuldin commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

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

Can't find any papers to compile :-(

StefanGuldin commented 5 years ago

Dear @trallard,

We created a new release on github (v1.0.1) to encompass the latest revisions. We have published the package on Zenodo and obtained the following DOI:10.5281/zenodo.2847906. Unfortunately, there appear to be some issues with compiling the .pdf and I am unsure how to proceed. I would appreaciate any advice.

Thank you,

Niamh

arfon commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

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

[WARNING] Could not convert image '/tmp/tex2pdf.597/279c0eb56ba260c2eb47e84a0963ff2ed937ca37.shtml': Cannot load file Jpeg Invalid marker used PNG Invalid PNG file, signature broken Bitmap Invalid Bitmap magic identifier GIF Invalid Gif signature :

HDR Invalid radiance file signature Tiff Invalid endian tag value TGA Invalid bit depth (104) [WARNING] Could not convert image '/tmp/tex2pdf.597/4c92490c05e42464422b1211c0dcf578b5cba966.shtml': Cannot load file Jpeg Invalid marker used PNG Invalid PNG file, signature broken Bitmap Invalid Bitmap magic identifier GIF Invalid Gif signature :

HDR Invalid radiance file signature Tiff Invalid endian tag value TGA Invalid bit depth (104) Error producing PDF. ! LaTeX Error: Cannot determine size of graphic in /tmp/tex2pdf.597/279c0eb56ba 260c2eb47e84a0963ff2ed937ca37.shtml (no BoundingBox).

See the LaTeX manual or LaTeX Companion for explanation. Type H for immediate help. ...

l.322 ...b56ba260c2eb47e84a0963ff2ed937ca37.shtml}

Looks like we failed to compile the PDF

arfon commented 5 years ago

@adrena-lab - please update the URLs for images in your paper.md to link to GitHub RAW URLs e.g. https://raw.githubusercontent.com/adrena-lab/Optimising-Light-Source-Positioning/Code/Figures/Figure1.png

StefanGuldin commented 5 years ago

Dear @arfon

Thank you very much. I will try that now.

Niamh

StefanGuldin commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

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

StefanGuldin commented 5 years ago

Dear @arfon,

I made those changes but am still encountering an error but it I cannot see what the error is. Again, any advice would be appreciated.

Thank you,

Niamh

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

StefanGuldin commented 5 years ago

Thank you!

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

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

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

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

StefanGuldin commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

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

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

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