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]: PyAstroPol : A Python package for the instrumental polarization analysis of the astronomical optics. #2693

Closed whedon closed 3 years ago

whedon commented 4 years ago

Submitting author: @hemanthpruthvi (Hemanth Pruthvi) Repository: https://github.com/hemanthpruthvi/PyAstroPol Version: v0.1.0 Editor: @pibion Reviewer: @aquilesC, @caldarolamartin, @mwcraig Archive: 10.5281/zenodo.4285762

: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/3d5f2d8bca0c68fa84302c3904e1638e"><img src="https://joss.theoj.org/papers/3d5f2d8bca0c68fa84302c3904e1638e/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/3d5f2d8bca0c68fa84302c3904e1638e/status.svg)](https://joss.theoj.org/papers/3d5f2d8bca0c68fa84302c3904e1638e)

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 & @caldarolamartin & @mwcraig, 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 @pibion 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 @caldarolamartin

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @mwcraig

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. @aquilesC, @caldarolamartin, @mwcraig 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 (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1086/599043 is OK
- 10.1117/12.2181397 is OK
- 10.1117/12.2233176 is OK
- 10.1201/b19711 is OK
- 10.1117/1.JATIS.4.4.048002 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 4 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

caldarolamartin commented 4 years ago

Hi. I cannot modify the check list and the link to accept is expired. @pibion could you please give me a hand to solve this? Thanks

caldarolamartin commented 4 years ago

Hi again. I've tried to tick the boxes with out success again. I have not accepted the review formally and the the link to do so (point 2 at the top) is not working. I believe this is the reason I cannot start with the review list check. @pibion, could you please take another look?

mwcraig commented 4 years ago

Hi, I also cannot check the boxes....

pibion commented 4 years ago

@caldarolamartin, @mwcraig are you signed in to github? You should only need to (1) subscribe to the issue and (2) be signed in to edit the checkboxes.

@arfon is that correct?

arfon commented 4 years ago

@whedon re-invite @caldarolamartin as reviewer

whedon commented 4 years ago

OK, the reviewer has been re-invited.

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

caldarolamartin commented 4 years ago

Yes! now it works. Thanks!

arfon commented 4 years ago

@mwcraig - it looks like you need to accept the invite to the the repository here https://github.com/openjournals/joss-reviews/invitations before you can click the checkboxes.

Screen Shot 2020-09-28 at 18 26 08

@caldarolamartin - I've re-invited you to the repository - please accept the invite at https://github.com/openjournals/joss-reviews/invitations .

pibion commented 4 years ago

Thanks @arfon , shoulda checked the whedon docs :/

caldarolamartin commented 4 years ago

Hi! I've just posted an issue with my review comments.

hemanthpruthvi commented 4 years ago

Hi, I have replied to the comment.

pibion commented 4 years ago

@caldarolamartin just wanted to make sure you saw this update from @hemanthpruthvi .

pibion commented 4 years ago

@hemanthpruthvi, I think the idea of an environment file is a good one since the dependencies are somewhat sensitive. @caldarolamartin , I've always used conda environment files for this. Do you have any other recommendations?

caldarolamartin commented 4 years ago

Hi @pibion, I've just closed the issue I opened for my review comments. Regarding the dependencies, I've also only used conda environment files, but I know virtualenv can do the same job.

aquilesC commented 4 years ago

@pibion and @hemanthpruthvi , I am in the middle of the review. I strongly suggest, as @caldarolamartin points out, to create requirement files and a setup file. This will make life much easier for other users. Sorry for the self-promotion, I've written a guide on setup files here, and a very short introduction to virtual environments here that can be used as a starting point.

Even though conda environments are very handy, they are particularly well suited for isolating complete work environments. I am not completely convinced conda environment files are meant to be distributed to a wider audience unless there are stringent needs (and this is not the case for PyAstroPol that relies on widely available libraries). I believe that a conda environment.yml file may create conflicts (conda environments are not particularly flexible). I would favor a requirements.txt file that makes explicit required (minimum) versions.

aquilesC commented 4 years ago

I have reviewed the paper. I have noted a lot of comments that can be used by the author to improve the quality of the code and the paper but that do not impact directly the decision of whether the program is ready to be published. However, there is a (short) list of issues at the bottom of this message that must be addressed in order to make the work, in my opinion, ready to be published.

The Paper

The paper stresses too much the limitations and too little the features. The first mentioned feature says:

However, a major caveat here is that, to computeMueller matrix the electric fields of all the rays will be added coherently after propagating through the system.

Even the features are handicapped in the description of the software instead of focusing on the strengths. Of course, this is a choice of the authors, but I do think that the impact can be related to how one decides to present the work.

Something that I have missed from the paper is how the program relates to other existing packages (if any). It can be that this is the first package addressing polarization, but the author also mentions comparing to Zeemax as the ground truth. I would replace several of the stated limitations to expand on a discussion about how this program fits in the community.

Writing

The writing can be improved. There are several sentences of the form

Majority of the optical surfaces in astronomical

Instead of

The majority of the optical surfaces in astronomical

Or

The package can be described as collection of the important objects

Instead of

The package can be described as a collection of the important objects

The Code

Installation

The readme file includes a list of modules that are used, such as os, and copy which are Python built-ins, while it also adds numpy and matplotlib which are external libraries and must be installed. It also mentions mpl_toolkits which is part of matplotlib and therefore can't be installed separately.

Installation instructions must be improved. A novice user will not be able to go through them. This can be quickly solved by: adding a requirements.txt file to the root of the project, and/or creating a setup.py file specifying dependencies and versions. The latter also helps with the instruction of adding the folder to the PYTHONPATH/

Documentation

The documentation is provided as a sphinx project, but sphinx is not listed in the requirements, therefore it can't be built by a user. Even though the documentation is on-pair right now with the code, since it has no visible continuous integration tools to keep the documentation always up to date, I would suggest either leaving the _build folder out of the repo and asking people to build the docs, or using tools such as ReadTheDocs to simplify the process.

Note: The readme points towards a html file in the docs folder which, on Github, renders as the source code and not as a website. This makes it very hard to explore the documentation online and/or offline, especially when relying on sphinx-autodoc (the documentation is built from the source code of the program and therefore is not explicitly written in the docs folder).

The index page of the documentation has very little information on how to get started. It is customary to include some general information, similar to the readme file, and link to specific points for the reader to be able to crack into the program.

The code itself is very well documented. Every method and class I have checked had appropriate docstrings, so compliments to the author for taking the time to do such a thorough work. That really pays off for anybody trying to learn from the code and, potentially, contribute to it.

Quality of the Code

The program performs the tasks that it needs to perform. However, inspection of the code leaves several rooms of improvement that relate to standards (but not requirements) of how Python programmers work.

For example, using from PyAstroPol import * is highly discourage, since this is prone to name clashes and makes it very hard to understand what is going to be available to the developer. This is done through the code everywhere, for example in the __init__ file every module is imported with import *. And this propagates to the example on the Readme, where matplotlib is never imported but the code still works. This behavior is also visible within the program files. For example, material.py uses numpy, but numpy is never imported.

Another topic is the non-adherence to PEP8. Again, these are guidelines, but that can ensure the long-life of a project by lowering the barrier to future contributors. I believe all methods use lowerCamelCase, but attributes of classes don't follow a regular pattern. For example, in coating.py there is self.Wavelength and self.iRI.

Examples

The examples are a great starting place. I think they are the best addition to the code since they very quickly allow a new user to get the gist of the program and start adding small modifications to it.

However, all the examples run into a RuntimeWarning because of a division by 0. This raises the concern of whether the code is actually performing the right calculation or if there are problems that may have an impact on the obtained values. I have no way of verifying the accuracy claims:

Results of the code have been verified against previous works (Pruthvi, Krishnappa, Belur, &Kadagattor (2018)), and ZEMAX© OpticStudio – a popular commercial software.

Materials Licenses (discussion)

The code re-distributes material properties as csv files under an MIT license. Many of this materials are taken from PhysRevB which is not necessarily compatible with the chosen license. I am not 100% sure whether there is a license incompatibility or whether people is free to redistribute data without consent. Any advice from @pibion @caldarolamartin or @mwcraig ?

Conclusions

The paper and the code are ready to be published provided that the author performs the following:

hemanthpruthvi commented 4 years ago

@aquilesC Thank you very much for the thorough review and the valuable comments, I am working on addressing them. Meanwhile, I want to quickly address the issue regarding the Material Licenses. If I understand correctly, there are two issues.

  1. Can the Material files be re-distributed, according to their existing licensing?
  2. If so, can they be re-distributed under MIT license?

For issue 1 : Albeit the information listed in the Material files originated in various publications, the raw files themselves are directly downloaded from https://refractiveindex.info/. The sources of the data are also listed there. The website's database is public domain via CC0 1.0 license i.e., whatever data hosted there is free to be used/distributed without seeking any consent.

image

For issue 2 : If re-distributing under MIT license is an issue, I propose either of two ways for resolution, whichever is plausible.

  1. Having two licenses : MIT -- for the code, and CC0 1.0 -- for the database, or
  2. Exclude the database from the scope of the existing license, and simply state -- "XXX data is downloaded from YYY, hosted under ZZZ license"
aquilesC commented 4 years ago

@hemanthpruthvi I have seen the license of refractiveindex.info, but I am not sure whether the author is actually entitled to re-distribute the data under a CC0 license. It would, for example, eliminate the citation to the original work with the real measurements. The sources of the website are papers not published under open access and I haven't seen information on APS regarding the distribution of datasets. I know, for example, that Comsol distributes some material properties but more recent measurements can't always be built-into the program, therefore they point you in the direction of how to get the dataset. In any case, this is a out of my depth regarding my understanding of copyright. I think the polite (and perhaps correct) thing to do is to add, in the materials folder, the citation of where the indexes come from (not the website, but the original work) since this is the best way of having a reproducible workflow in the future. You could add a Readme file directly in that folder.

hemanthpruthvi commented 4 years ago

@aquilesC I understand and I agree. Accordingly, I have added a Readme file in the materials folder that refers to original sources.

hemanthpruthvi commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

hemanthpruthvi commented 4 years ago

@aquilesC Thank you again for the review. Please find the updated contents.

The Paper

I have put a little more emphasis on the features and added some content as to how the code fits in the community. I have also re-written parts of the paper.

I believe that an explanation is needed regarding the writing choices. As I have mentioned in the updated paper, I do not know of an open-source software with the features of PyAstroPol. My counterparts in the field and I have followed the approach that is mentioned in the paper: make instrument specific models, use commercial software, or a combination of both. Hence, my first instinct was to compare with the commercial software that we have already been using, and the emphasis on the limitations is probably a reflection of that process.

The Code

I have added requirements.txt to the package. I have also elaborated on the installation instructions.

I have hosted the documentation at ReadTheDocs and provided the link in the README. I am grateful for your compliments regarding the code documentation. I am glad that you took notice of the efforts.

I have modified the examples to reflect the recommended usage of import. Now, it is import PyAstroPol as pap. I have some additional development plans which might require some restructuring. I will consider changes to the internal code at that time.

I have tried to adhere to lowerCamelCase for the most part, and all the methods follow that. Unfortunately, the attributes do deviate from this because I chose names/abbreviations which are popularly in use already. That is the case with the attributes such as self.rs or self.Wavelength. In case of self.iRI, the alternative was self.iRefractiveIndex which, I believe, would be needlessly longer.

Runtime warning

I have now suppressed the runtime warning. The code uses array operations instead of conditional loops for the sake of performance. Hence, these warnings are inevitable, and I have taken special care to deal with such cases. It can be found in the form of occasional nan_to_num operations, and so the output results are proper.

The example named 11_KTTCoelostat is provided so that the user can verify the accuracy claims. It compares the outputs of PyAstroPol and Zemax OpticStudio. Unfortunately, this is possible only if the user has the software license, and an additional package PyZDDE (Python-Zemax interface).

aquilesC commented 4 years ago

@hemanthpruthvi , good work! I've just checked the manuscript, and it sounds much better with the positive aspects of your software stated first, and also the improvements to your code and examples are definitely a good addition.

I have marked all the checkboxes, @pibion , I think the work is ready to be published, I believe my review concludes, but please feel free to ping me if I can do anything else for you.

pibion commented 4 years ago

@aquilesC many thanks!

pibion commented 4 years ago

@caldarolamartin , @mwcraig, do you have an ETA on when you might be able to spend time on your reviews?

caldarolamartin commented 4 years ago

Hi @hemanthpruthvi! I have gone through the revised manuscript and code and I agree with Aquiles, it is improved and ready for publication.

I have marked all my checkboxes, @pibion, I also think paper is ready to be published.

pibion commented 4 years ago

@caldarolamartin thanks for looking through the revised submission!

mwcraig commented 4 years ago

@pibion -- I'll get more comments in tonight.

@hemanthpruthvi -- neat looking package and thanks for the improvements so far!

  1. My main concern at this point is installability of the package. Would you be amenable to adding a small setup.py file to your repository? I'm happy to help you get that written. I think many users will be hesitant to modify PYTHONPATH as it is no longer a standard practice and may have unintended side effects.
  2. With the addition of a line or two of code in each of the Python files in PyAstroPol defining which things get import with * you could ensure that when users import your package they get only your modules, not additional ones like numpy, etc. I'm happy to help you do this too if you are amenable to doing it.
  3. The addition of a requirements file is excellent! It looks like it has two .txt extensions, though, so you should probably rename it to requirements.txt.
  4. There are a few folders in the repo that you could get rid of by adding them to .gitignore then deleting the folders and commit those changes. The ones I have in mind are any .ipynb_checkpoints and __pycache__ folders.

The spirit of this is to help you make it as easy as possible for potential users to install and try out your package when the paper is published. The package itself is very interesting!

mwcraig commented 4 years ago

My only remaining concerns are the ones listed above. I'm happy to help get those fixed with some PRs if you want.

hemanthpruthvi commented 4 years ago

@mwcraig Thank you very much for the review and the offer to help.

  1. I very much agree with you, and with @aquilesC and @caldarolamartin as well, regarding the installability. I absolutely intended/intend to have a setup.py so that the users can install the package by simply using pip, like all other packages. The reason it is not there at the moment is something to do with the user data. The commercial software that is widely used for these purposes creates two directories when installed: root directory -- for the program files, and user directory -- for the files to be accessed by the user. In my package, ./Examples/ and ./Materials/ come under the user data, some of which will also be accessed by the program in the runtime. I think the simpler solution is to just keep one directory, but then it is not desirable to have it somewhere in site-packages. Hence, I have resorted to present installation scheme. I was hoping to get some user input and features in, so that I can decide if I should take the trouble to have two directories. I very much welcome if you have any ideas in that regard.

  2. I have modified the file __init__.py. Now, other packages like numpy will not be imported when PyAstroPol is imported. accordingly.

  3. Taken care.

  4. Taken care.

aquilesC commented 4 years ago

@hemanthpruthvi , what you mention does not have a unique answer, it will depend a bit on what you expect the users to achieve. There are two things you can do. One is, during installation create a folder in the $HOME directory (or create it the first time the package runs) and put the examples and materials in that folder, so the user can easily modify them.

You can also use a variable (environmental or explicit, something like: os.environ['pyastropol_folder']) to point to the directory where the materials are stored.

You can also opt for installing your package in development mode (pip install -e or python setup.py develop which will add to the PATH without the need to ask the user to explicitly mess with the Pythonpath, and you'll get the extra advantage of working inside virtual environments out of the box.

Although I think any of the suggestions above can be good additions from a user perspective, I don't think they are part of the review process itself.

mwcraig commented 4 years ago

@hemanthpruthvi -- thanks for the explanation about #1, that makes much more sense. One suggestion (though I'm finishing out my review indicating this is ready to go) is to add a note to your README asking users to provide feedback on this.

hemanthpruthvi commented 4 years ago

@aquilesC Thank you very much for all your suggestions! I will be considering them when revising the installation scheme.

@mwcraig I have appended README with a request to the users about the feedback regarding the installation scheme.

pibion commented 4 years ago

@mwcraig have @hemanthpruthvi's changes to the README satisfied the installation requirement, in your view?

pibion commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

mwcraig commented 4 years ago

@mwcraig have @hemanthpruthvi's changes to the README satisfied the installation requirement, in your view?

Yes, given the special installation situation here.

pibion commented 4 years ago

Hi @hemanthpruthvi it looks like we're nearly ready to publish. I'm reading over the paper and have a few comments. I haven't finished reading the paper yet but wanted to get these to you quickly, I'm expecting to finish a bit later today!

Questions about the paper.

Suggestions for the text - feel free to ignore or incorporate at your discretion.

pibion commented 4 years ago

@hemanthpruthvi here are the rest of my comments on the paper.

Questions about the paper

Suggestions for the text

hemanthpruthvi commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

hemanthpruthvi commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

hemanthpruthvi commented 4 years ago

@pibion Thank you very much for your feedback. I have incorporated some of your suggestions for the text in the updated manuscript.

Answers to questions (in the same order) :

pibion commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

pibion commented 4 years ago

@hemanthpruthvi okay, thanks for these updates! I have only one question remaining:

I'm still not sure what you mean when you say "It can be easily distributed along with the polarimetric calibration routines." Do you mean, "It can be easily distributed along with the polarimetric calibration routines for a particular telescope?"