[REVIEW]: GB_code: A grain boundary generation code

[whedon]

Submitting author: @https://github.com/oekosheri (raheleh Hadian)
Repository: https://github.com/oekosheri/GB_code
Version: v1.0.0
Editor: @labarba
Reviewer: @vyasr, @trallard
Archive: 10.5281/zenodo.1433530

Status

Status badge code:

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

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@vyasr & @trallard, 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 @labarba know.

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

Review checklist for @vyasr

Conflict of interest

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

Code of Conduct

• I confirm that I read and will adhere to the JOSS code of conduct.

General checks

• Repository: Is the source code for this software available at the repository url?
• License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
• Version: Does the release version given match the GitHub release (v1.0.0)?
• Authorship: Has the submitting author (@https://github.com/oekosheri) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

• Installation: Does installation proceed as outlined in the documentation?
• Functionality: Have the functional claims of the software been confirmed?
• Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

• A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
• Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
• Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
• Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
• Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
• Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

• Authors: Does the paper.md file include a list of authors with their affiliations?
• A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
• References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @trallard

Conflict of interest

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

Code of Conduct

• I confirm that I read and will adhere to the JOSS code of conduct.

General checks

• Repository: Is the source code for this software available at the repository url?
• License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
• Version: Does the release version given match the GitHub release (v1.0.0)?
• Authorship: Has the submitting author (@https://github.com/oekosheri) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

• Installation: Does installation proceed as outlined in the documentation?
• Functionality: Have the functional claims of the software been confirmed?
• Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

• A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
• Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
• Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
• Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
• Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
• Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

• Authors: Does the paper.md file include a list of authors with their affiliations?
• A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
• References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

[whedon]

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

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

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

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

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

@whedon commands

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof 📄 <--

[labarba]

👋 @vyasr, @trallard — here's where the action happens. Find your checklist above, and post here with any questions or comments. Thanks for volunteering to review!

[vyasr]

@labarba I'm not quite sure how to review aspects of this repository. It's written in Python, but it's not really a Python package, it's a collection of scripts for a particular purpose. There is no Python API, the intent is just to run these scripts on the command line. That being the case, the normal aspects of a Python package (a setup.py for installation, a Python API, etc) don't really apply. I can request tests for the command line API and some versioning on the repository itself, but since the package won't be setup.py installed the version would also just be viewed by looking at the repository source.

Should I just treat this as a command line tool, and give advice accordingly? Or should I be providing suggestions on how to expose/improve a Python API and follow more standard Python packaging guidelines? @oekosheri do you want/see a use for making this tool scriptable within Python rather than as a standalone set of scripts?

[oekosheri]

True, I have not provided a setup.py but this is definitely a python package.
Both main scripts are modules (collection of functions and classes) as well as command line scripts because of the main functions in the end that parses input arguments. Why I have chosen this structure: historical reasons + the underlying physics + preference by the scientists who use it.
If you think I should use a setup.py, I can add it of course. If you have suggestions as to how it should be written, let me know please. If you meant something else that I didn't understand correctly please clarify it a bit more to me.

[vyasr]

You understood my comment correctly. The underlying code is cleanly divided into functions and classes, and you are correct that in the strict sense the repository is a Python package since it is importable thanks to the init.py file. I was mostly alluding to the fact that it is not immediately installable, and that the intended API appears to be entirely command line based. You've put together a nice implementation of a cool scientific application, I'm just trying to get a sense for how to make it most useful.

My recommendations will depend a little bit on how you anticipate people making use of the code. If your intended usage is purely from the command line, then I think my comments regarding how to appropriately evaluate this apply because there really isn't much of an API other than simply executing the scripts as you demonstrate in your README. On the other hand, if you want people to be able to interface directly with the Python functions that you provide (in my opinion, you should allow that since it gives users much more flexibility and they might find use-cases you didn't anticipate), then there are a few things you would want to do. Your repository should be restructured in the form of a standard Python package (see here for example) and you would use a setup.py to make it installable and accessible. If you want to expose a command line interface, the way to do it in that context would be using setuptools entry_points (if you're not familiar, you can see the official docs as well as a few basic tutorials). Even if you don't provide a Python API, making your scripts available on the command line in this fashion would make it easier for people to incorporate the commands into their own (Python, bash, etc) scripts.

Let me know your thoughts on these ideas and I can tailor my feedback accordingly.

[oekosheri]

Thank you for your help.
To answer your question:
Currently, the more classic scientists use it via the Linux terminal and the younger scientists use the Jupyter notebooks. They can import different modules for the functionality they need and there are also additional functions in the notebooks. As far as the further development goes, I see only myself as the only developer of this code.

1- you say: "Your repository should be restructured in the form of a standard Python package ", I do not quite understand how. Do you mean to separate the script part (main functions) of the codes from the modules (functions/class)?
If your answer is yes, I must say I would prefer to keep them as is because of historical preference.
If your answer is different, please let me know.

2- I think it would probably make more sense to me to use the "entry point" solution for the setup.py and keep the scripts as they are.

[vyasr]

As you pointed out, your code already is a package. The standard structure for Python packages, however, is to have all the code in a subdirectory of the repository:

README
setup.py
requirements.txt
GB_CODE/init.py
GB_CODE/csl_generator.py
GB_CODE/gb_generator.py

With this structure, setup.py will be able to install the GB_code modules using a very standard setup. I think keeping the main function in the same file as the modules is fine, it doesn't cause any problems at the moment. This would also make it easier to automate testing.

The idea with entry points is that someone from anywhere on their filesystem would be able to type gb_generator arg1 arg2 ..., for example. This would make it easy to insert your code into other scripts.

[oekosheri]

Yes, that's right. thanks.
So I'm trying re-structuring and I have a question. I put the two scripts/modules in gb_code directory. My setup.py file looks like this:

 from setuptools import setup
  
packages = [
    'gb_code',
       ]
setup(
    name='GB_code',
    version='0.1.0',
    author='R.Hadian',
    author_email='[email protected]',
    packages=packages,
    entry_points = {
        'console_scripts': [
            'csl_generator = gb_code.csl_generator:main',
            'gb_generator = gb_code.gb_generator:main',
        ],
    },
    url='https://github.com/oekosheri/GB_code',
    license='LICENSE',
    description='A GB generation code',
    long_description=open('README.md').read(),
)

It copies the executable scripts in python bin, so very good but it does not copy them in site-packages.
if instead of setuptools I use distutils.core the modules will be installed in site-packages but the entry points are not recognised. Do you know what may be the issue?

Thanks in advance.

[oekosheri]

A follow up on my previous post:
I triedpip install . and that did the task. i.e. package was copied to my python site-packages and executables to python bin. I wonder if this is the best practice though. If you know of a better way, let me know.

[vyasr]

I don't know exactly what caused the issue that you were having, but in general setup.py is known to have some buggy behavior in odd cases like that, so I don't think that you're doing anything wrong. You could try on a different python version if you have conda/virtualenv handy and see how that behaves, but I wouldn't be too worried.

[oekosheri]

Ok, Thanks, So I updated the repository now.

[oekosheri]

Sorry to bother but I was wondering whether the review process is progressing. Do I need to do make any other changes at this point?
I have a deadline for introducing my work and want to know whether the review process will be done before that.

[labarba]

👋 @vyasr, @trallard — could you give us an update on your timeline for review?

[trallard]

sorry, I have blocked some time on Monday-Tuesday to do so so should be able to update then

[vyasr]

Hi sorry I had a couple of things come up after my flurry of activity two weeks ago. I will get to this tomorrow.

[vyasr]

Thanks for those changes! Sorry for the delay. Here is my next set of comments.

• Python functions should add some function/class level documentation of the APIs. The functions all have docstrings, which is helpful, but they should also have some description of what the arguments are and what the object types are (floats, lists, some internal class, etc).
• Since you have a setup.py already and your project is pure Python, you can make it even easier to install by uploading your project to PyPI (or even Anaconda cloud). For more information on this, see the Python Packaging Guide, and specifically the guide on managing and packaging your own project.
• Can you add some automated tests to make sure that the outputs are as expected? I can run your code, but I don't have great topical knowledge of CSL so it would be helpful if you added a tests folder that would run the generator and then check that the output files are what you would expect from theory. Ideally, there would be automated tests, for which you can use the Python unittest framework, or pytest if you want something more sophisticated. The notebooks you have are good, but it would be nice to have. I'd say it's a lower priority than documentation, but it also shouldn't be too hard to use nbconvert to make the notebooks into scripts, place it in a unittest class, and add some assertions.
• Since you're using the README as your primary source of documentation, you should add a small section indicating how people can contribute (e.g. "Submit a PR to this repository). Even if you don't anticipate contributors, it's good to have that to lower the barriers, you never know when an interested user might provide you with something useful!

[oekosheri]

Thank you for your comments.

• I will add more description to the docstrings to describe the parameters a bit more but I hope I don't have to necessarily have full descriptions the same way python does for example. That would take a very long time.
• I think I can consider this later but for now the installation is rather easy and it seem to work fine.
• The reason I added the Jupyter notebooks to this directory is that they technically show the functionality aside from the command line. So I thought that should serve the purpose of testing as well. If I must add the unittest I will but If the notebooks suffice that would be my preference.
• For a variety of reasons I do not want to accept contribution to the code at this point. I plan to expand this code and when the time comes I will add a contribution section to the README. I hope this is alright.

I submitted the code more than two months ago and have a deadline for presenting it before leaving my institute. Prior to submitting it, I have put quite some time and effort into making the functionality and presentation of it match the needs of current/future users. I really appreciate the fact that I could improve the installation procedure following your comments last time but I also have a time limit now. Aside from completing the docstrings which I will try to finish tomorrow, I would appreciate it if you could test the code now as is.

[oekosheri]

@labarba @vyasr @trallard I have updated the repository after adding more to the doctrings of the csl_generator script. I have checked the requirement for the documentation in JOSS and think my code satisfies it for the most part. If there is another necessary step that I'm missing please let me know.
Aside from this, I would like to know the timeline for the review process as I must present the code before my departure.

[labarba]

👋 @vyasr @trallard — Can you update me on the status of your review? Perhaps you could give us an estimated time for completing the review, or an assessment of the state of the submission (how close to acceptance it is)?

Thank you for volunteering on this adventure in new-wave publishing!

[vyasr]

That's fair enough. I can review as is.

The only remaining requirement I have is that you update the setup.py script to have install_requires = ['numpy']. Currently if you try to install this into a clean environment it will fail to run. The requirements.txt file is helpful, but users using pip/setup.py based installation should get the dependencies automatically. pandas and matplotlib are not necessary, since they're just for the notebooks, not the package itself (as far as I can tell).

Regarding not accepting contributions, as far as I am concerned that is fine. @labarba may have more to say on that front, but I will check that box for now and can revise if needed.

Once the setup change is made I can approve.

[oekosheri]

@vyasr thanks! I updated setup.py file with the required numpy version.
@labarba please let me know whether I should add anything to README regarding the contributions. As stated before, my preference is that for now I will not accept contributions. Later on I can change that.

[trallard]

Hi @oekosheri I am doing the review right now and I will soon be opening some issues (and referencing here for completeness/openness
I think @vyasr has been rather thorough so far

• Specify Python version requirements: this is helpful as you never know what environment your users have
• Contributions: I think it is appropriate to make it clear that you are not accepting contributions at the moment this makes it easier for you in the sense of not having to deal with unsolicited contributions and avoids user disappointment

[trallard]

• Satement of need: there is a clear statement in the paper, I wonder if this could be transferred to the README.md (or a short version of it)
• Release tag: although the packages ia versioned as v1.0.0 there is not such a tag in the repository

I think that covers the missing gaps/issues I encountered, so once these issues are resolved I would be happy to complete the review from my side. (apologies for the long time)

Re tests: although I appreciate that the notebooks are there to demonstrate the functionality of the code these should ideally be accompanied with unit tests. I do however, appreciate this means extra effort and time commitment so I would suggest adding that as a work in progress or consider adding tests to cover the different methods in the package. progressively to ensure things work as expected.

[oekosheri]

Thank you @trallard
I specified the version of python in setup.py (additionally in README).
I detailed the status on contribution in README.
Regarding the Jupyter notebooks: I have mentioned them in the README and have linked to them.
I can maybe put them outside Test and on the root directory if they are not visible. What is your suggestion?

[trallard]

Thanks a lot for the quick actions @oekosheri.

I closed the notebooks issue as I realized that I did not notice the link to the notebooks.
But for now I'd leave them where they are

[whedon]

👉 Check article proof 📄 👈

[oekosheri]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

👉 Check article proof 📄 👈

[oekosheri]

@labarba Sorry for testing a few times. Does the last version look acceptable?

[labarba]

Something is wrong with the DOI links. For example, the second reference links to: https://doi.org/https://doi.org/10.1103/PhysRevMaterials.2.043601

Please clean up the BibTeX entries so they resolve properly.

[oekosheri]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

👉 Check article proof 📄 👈

[oekosheri]

It must be correct now. I removed the https. Please let me know.

[labarba]

Great!

The next step is for you to make an archival copy of the full repository in a service like Zenodo, get a DOI, and tell us the archive DOI here so we can add it to the publication.

[oekosheri]

Thanks @labarba, so eventually I managed to integrate the repo with Zenodo and it created a badge with Doi.

Is it correct?

[labarba]

That DOI resolves correctly, yes. But note that the authors are listed as "oekosheri; Arfon Smith" in the Zenodo entry. Probably you had a pull-request from Arfon that you merged, and Zenodo is taking the authors from the committers, automatically. You need to manually edit the metadata to display the correct author list in Zenodo.

[oekosheri]

I edited it, thanks. Do I need to do any thing else?

[labarba]

Lastly, I do have a question for you. You've only listed yourself as author in the Zenodo archive. We only see yourself as a committer in the code repository. Yet you have three authors in the JOSS paper. I'm a little worried about courtesy authorship here. The last author is the Director of the Computational Materials Design department at MPIE, and the second author is the group leader of Adaptive Structural Materials. Can you look at the JOSS authorship guide, reflect on this (possibly discuss with your listed co-authors) and make a final determination that the author list is what you all want?

From the JOSS guidelines:

Authorship

Purely financial (such as being named on an award) and organizational (such as general supervision of a research group) contributions are not considered sufficient for co-authorship of JOSS submissions, but active project direction and other forms of non-code contributions are. The authors themselves assume responsibility for deciding who should be credited with co-authorship, and co-authors must always agree to be listed. In addition, co-authors agree to be accountable for all aspects of the work.

[oekosheri]

I am the sole contributor of the code, which is a collection of work I did in the last couple of years under the active direction of the other two authors. So I can update Zenodo author list and include them as well.

[labarba]

It is your decision to make.

[oekosheri]

Thank you. I already updated Zenodo.

[labarba]

@whedon set 10.5281/zenodo.1433530 as archive

[whedon]

OK. 10.5281/zenodo.1433530 is the archive.

[labarba]

@arfon : This submission is accepted and ready to be published.

@vyasr, @trallard : Thank you for your valuable contribution to JOSS by reviewing this publication!

[oekosheri]

Thank you all for your help! :) @labarba @vyasr @trallard @arfon

[arfon]

@vyasr, @trallard - many thanks for your reviews here and to @labarba for editing this submission ✨

@oekosheri - your paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00900 ⚡ 🚀 💥

[whedon]

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](http://joss.theoj.org/papers/10.21105/joss.00900/status.svg)](https://doi.org/10.21105/joss.00900)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.00900">
  <img src="http://joss.theoj.org/papers/10.21105/joss.00900/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: http://joss.theoj.org/papers/10.21105/joss.00900/status.svg
   :target: https://doi.org/10.21105/joss.00900

This is how it will look in your documentation:

We need your help!

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:

• Volunteering to review for us sometime in the future. You can add your name to the reviewer list here: http://joss.theoj.org/reviewer-signup.html
• Making a small donation to support our running costs here: https://www.flipcause.com/secure/cause_pdetails/Mjk3Nzk=