search

openjournals / joss-reviews

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

[REVIEW]: GIMS: Graphical Interface for Materials Simulations #2767

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @kokookster (Sebastian Kokott) Repository: https://gitlab.com/gims-developers/gims Version: v1.0.2 Editor: @jgostick Reviewers: @marshallmcdonnell, @jgostick Archive: 10.5281/zenodo.4386436

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

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

@marshallmcdonnell & @arose, 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 @jgostick 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 @marshallmcdonnell

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @jgostick

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @marshallmcdonnell, @arose, @marshallmcdonnell 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 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- None

MISSING DOIs

- 10.1016/j.cpc.2009.06.022 may be a valid DOI for title: Ab initio molecular simulations with numeric atom-centered orbitals
- 10.1088/0953-8984/26/36/363202 may be a valid DOI for title: Exciting: a full-potential all-electron package implementing density-functional theory and many-body perturbation theory
- 10.1016/j.commatsci.2010.05.010 may be a valid DOI for title: High-throughput electronic band structure calculations: Challenges and tools
- 10.1007/978-1-4842-2677-3_5 may be a valid DOI for title: pytest

INVALID DOIs

- None
whedon commented 3 years ago

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

marshallmcdonnell commented 3 years ago

@jgostick Small question, it looks like I've been added twice to the review above:

Reviewer: @marshallmcdonnell, @arose, @marshallmcdonnell

1) Does this matter? 2) Is this something that I need to use whedon to remove?

Thanks in advance!

jgostick commented 3 years ago

I'm still leaning how to interact with the whedon thing. I thought I'd finally got it right this time :-( Anyway, it doesn't matter much, at the end of the day I just look at the check boxes above. I did remove the second set of check boxes for you though.

marshallmcdonnell commented 3 years ago

Awesome, thanks!

marshallmcdonnell commented 3 years ago

@Kokookster really like your software package, see it as one of many needs to bring web-based solutions in the materials modeling and simulation domain! Great documentation and clean website design, very intuitive. I have went ahead and opened the following issues in the GIMS repository for my request of minor revisions. Besides that, I don't see any issues publishing in JOSS.

Minor revisions

I also added a "suggested revision" and merge requests that do not bar acceptance for me but think would be good to address / include.

Suggested revisions

I figured out the installation via "dockerizing" and the interface worked with everything I "threw at it" :grin:

Tests ran as expected for me (followed the GitLab CI yaml for instructions). I encourage expanding the test suite but doing great so far.

My biggest issues are Python application API documentation, contribution guidelines, comparison to other similar softwares in the paper, and resolving the license clash in the OrthographicTrackballControls module.

Let me know if you have questions about these issues and happy to engage in a discussion.

jgostick commented 3 years ago

Hi @marshallmcdonnell, thanks for a terrific review!

Kokookster commented 3 years ago

Hi @marshallmcdonnell, Thanks for your kind words and your constructive review! All points are really helpful. Thanks even for fixing small typos and adding the Docker! Will start to address your MRs, today.

jgostick commented 3 years ago

Hi @Kokookster it's been about 3 weeks since you checked in here...how are the revisions coming along?

Kokookster commented 3 years ago

Hi @jgostick, Hopefully, I addressed all the issues opened by @marshallmcdonnell with the following MRs: https://gitlab.com/gims-developers/gims/-/merge_requests/30 https://gitlab.com/gims-developers/gims/-/merge_requests/28 https://gitlab.com/gims-developers/gims/-/merge_requests/26 https://gitlab.com/gims-developers/gims/-/merge_requests/25 https://gitlab.com/gims-developers/gims/-/merge_requests/24 One MR is still open and waits for the go by the co-authors, but expect this by the end of this week. Haven't got a review by @arose. Hope I didn't miss anything.

jgostick commented 3 years ago

Hi @arose, how is your review coming along? The authors have been busy addressing the comments of the other reviewer so will be ready to receive yours soon.

marshallmcdonnell commented 3 years ago

Hello @jgostick and @Kokookster, Sorry for the delay in response, I have been out for the past ~2 weeks and just seeing the conversation. Yes, @Kokookster has been doing an excellent job addressing all issues I raised! I have tried my best to monitor the merge requests and updating my checklist above. I only have one more issue and the draft MR @Kokookster has already appears to address it.

So, yes, I will be ready for full approval once that is merged :+1:

Thanks for your patience with me on the review (my first JOSS review) and all the hard work!

jgostick commented 3 years ago

It seems that @arose has disappeared. I have emailed him but no reply. I am sorry for the delay. I will see about another reviewer. I am unable to review this myself.

Kokookster commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

marshallmcdonnell commented 3 years ago

Great work, @Kokookster, thank you for addressing all my comments! @jgostick I am satisfied that my review has been fully addressed and believe the work is ready to be published in its current state.

I understand that we are still trying to find a second reviewer and there will be edits based on the second review. I'll be happy to have another look after any edits from the outcome.

Let me know if there is anything else needed on my end in the meantime! :+1:

jgostick commented 3 years ago

@whedon remove @arose as reviewer

whedon commented 3 years ago

OK, @arose is no longer a reviewer

jgostick commented 3 years ago

@whedon list reviewers

whedon commented 3 years ago

Here's the current list of reviewers: https://bit.ly/joss-reviewers

jgostick commented 3 years ago

@whedon assign @marshallmcdonnell as reviewer

whedon commented 3 years ago

OK, @marshallmcdonnell is now a reviewer

jgostick commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

jgostick commented 3 years ago

Hi @Kokookster, I decided to perform the review myself since you've been waiting so long, and @marshallmcdonnell did such a thorough job. I can confirm that I am able to check off all the boxes above. I even installed node.js and npm so that I could build your code and all went fine. My only comment is that the installation instructions seem to wander off near the end. The beginning mentions that two steps are needed (install npm then pip), but I was able to open the index.html file and get things basically working without the pip part. I Think the python install was only required for developers so they can see any changes they make in real time without having to build the npm package everytime...is that correct? In any case, you might want to update the readme with a bit more details in this regard. I might also suggest that the various modules offer some way to preload some existing files, so users can poke around immediately (like you do on your demo site), but this is not necessary of course.

So, I'm now happy with this submission and will begin the process of accepting it.

jgostick commented 3 years ago

@whedon check references

jgostick commented 3 years ago

Here is a list of what to do next:

whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- None

MISSING DOIs

- 10.1016/j.cpc.2009.06.022 may be a valid DOI for title: Ab initio molecular simulations with numeric atom-centered orbitals
- 10.1088/0953-8984/26/36/363202 may be a valid DOI for title: Exciting: a full-potential all-electron package implementing density-functional theory and many-body perturbation theory
- 10.1016/j.commatsci.2010.05.010 may be a valid DOI for title: High-throughput electronic band structure calculations: Challenges and tools
- 10.1007/978-1-4842-2677-3_5 may be a valid DOI for title: pytest

INVALID DOIs

- None
jgostick commented 3 years ago

@Kokookster you'll also have to clean up the references...they are all missing doi's. Whedon has magically suggested 4 of them, but you have 6 papers in the bibliography.

Kokookster commented 3 years ago

Hi @Kokookster, I decided to perform the review myself since you've been waiting so long, and @marshallmcdonnell did such a thorough job. I can confirm that I am able to check off all the boxes above. I even installed node.js and npm so that I could build your code and all went fine. My only comment is that the installation instructions seem to wander off near the end. The beginning mentions that two steps are needed (install npm then pip), but I was able to open the index.html file and get things basically working without the pip part. I Think the python install was only required for developers so they can see any changes they make in real time without having to build the npm package everytime...is that correct? In any case, you might want to update the readme with a bit more details in this regard. I might also suggest that the various modules offer some way to preload some existing files, so users can poke around immediately (like you do on your demo site), but this is not necessary of course.

So, I'm now happy with this submission and will begin the process of accepting it.

Thanks @jgostick !

The python installation is mandatory for a working GIMS application. The static part of GIMS will still work without any running flask, however the server requests will remain unanswered, which is why you don't see the preloaded geometry from the demo site. The GIMS application should look and work like the demo page, if everything is installed properly. The easiest way to install everythin is using the docker, which was added by @marshallmcdonnell (Thanks again!).

Kokookster commented 3 years ago

@whedon check references

Kokookster commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.cpc.2009.06.022 is OK
- 10.1088/0953-8984/26/36/363202 is OK
- 10.1088/1361-648X/aa680e is OK
- 10.1016/j.commatsci.2010.05.010 is OK
- 10.1186/1758-2946-4-17 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago

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

jgostick commented 3 years ago

Hi @Kokookster , can you address the rest of the checkboxes in my comment above? Get a doi, tell me the version number, double check author affiliations, etc?

Kokookster commented 3 years ago

Hi @jgostick,

Sorry for the delay! Here are the requested things:

I checked all the ORCIDs and Names of the authors. They should correspond to an existing person and be consistent. Let me know if anything is missing!

volkerblum commented 3 years ago

Hi @jgostick and @Kokookster - Happy 2021! I stayed silent earlier since @Kokookster handled everything expertly ... just wanted to carefully ask if anything is missing for the paper from our (author) side or if we are in principle all set. No problem if all set (I thought we are), just wanted to make sure. Thank you very much for all of this! Best wishes, Volker

jgostick commented 3 years ago

Sorry for the delay...just emerging from vacay mode. I think things are ready now. I will process shortly.

jgostick commented 3 years ago

@whedon set 10.5281/zenodo.4386436 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.4386436 is the archive.

jgostick commented 3 years ago

@whedon set v1.0.2 as version

whedon commented 3 years ago

OK. v1.0.2 is the version.

jgostick commented 3 years ago

@whedon accept

whedon commented 3 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 3 years ago

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

Can't find any papers to compile :-(

whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.cpc.2009.06.022 is OK
- 10.1088/0953-8984/26/36/363202 is OK
- 10.1088/1361-648X/aa680e is OK
- 10.1016/j.commatsci.2010.05.010 is OK
- 10.1186/1758-2946-4-17 is OK

MISSING DOIs

- None

INVALID DOIs

- None
jgostick commented 3 years ago

I don't understand the failed paper compilation thing. An EiC will drop by to finalize the acceptance, so hopefully they'll have some insight.

jgostick commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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