openjournals / joss-reviews

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

[PRE REVIEW]: Lightshow: a Python package for generating computational x-ray absorption spectroscopy input files #4888

Closed editorialbot closed 1 year ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@x94carbone<!--end-author-handle-- (Matthew Carbone) Repository: https://github.com/AI-multimodal/Lightshow Branch with paper.md (empty if default branch): joss-manuscript Version: v0.0.15 Editor: !--editor-->@ppxasjsm<!--end-editor-- Reviewers: !--reviewers-list-->@maurov<!--end-reviewers-list-- Managing EiC: Kevin M. Moerman

Status

status

Status badge code:

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

Author instructions

Thanks for submitting your paper to JOSS @x94carbone. Currently, there isn't a JOSS editor assigned to your paper.

@x94carbone if you have any suggestions for potential reviewers then please mention them here in this thread (without tagging them with an @). In addition, this list of people have already agreed to review for JOSS and may be suitable for this submission (please start at the bottom of the list).

Editor instructions

The JOSS submission bot @editorialbot is here to help you find and assign reviewers and start the main review. To find out what @editorialbot can do for you type:

@editorialbot commands
editorialbot commented 2 years ago

Hello human, I'm @editorialbot, a robot that can help you with some common editorial tasks.

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

@editorialbot commands

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

@editorialbot generate pdf
editorialbot commented 2 years ago

Checking the BibTeX entries failed with the following error:

No paper file path
editorialbot commented 2 years ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.06 s (725.4 files/s, 107935.1 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          18            679           1279           2571
reStructuredText                13            221            243            217
YAML                             2             69             43            167
TOML                             1              6              6             83
Bourne Shell                     4             17              7             69
Jupyter Notebook                 1              0            471             44
DOS Batch                        1              8              1             27
make                             1              4              6             10
JSON                             1              0              0              1
-------------------------------------------------------------------------------
SUM:                            42           1004           2056           3189
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 2 years ago

Failed to discover a Statement of need section in paper

editorialbot commented 2 years ago

:warning: An error happened when generating the pdf. Paper file not found.

matthewcarbone commented 2 years ago

@editorialbot set joss-manuscript as branch

editorialbot commented 2 years ago

Done! branch is now joss-manuscript

matthewcarbone commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

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

matthewcarbone commented 2 years ago

@editorialbot set v0.0.15 as version

editorialbot commented 2 years ago

I'm sorry @x94carbone, I'm afraid I can't do that. That's something only editors are allowed to do.

matthewcarbone commented 2 years ago

Editors, I hot fixed the install to version-lock the Pymatgen dependency, since somehow a more recent version of Pymatgen breaks only some of the materials in our CI pipeline. New version is v0.0.15.

Kevin-Mattheus-Moerman commented 2 years ago

@editorialbot set v0.0.15 as version

editorialbot commented 2 years ago

Done! version is now v0.0.15

Kevin-Mattheus-Moerman commented 2 years ago

@x94carbone thanks for this submission. I have just added the query scope label here. This means our editorial board will review if this work is in scope for JOSS. Although extensive enough in terms of lines of code (we set a rough bar at around 1000 lines of code) this submission is on the small side. Furthermore, as can be seen in our submission requirements, "single functionality" packages are usually not in scope. From studying your repository and paper it looks to me like this work is a "generator of input files", as such this software perhaps classifies as a "single functionality" package. It does look like it is quite a technical challenge that is being solved, as generating these files sounds non-trivial. Hence I need the help of the board to review this in a bit more detail. Feel free to add some comments here too if you'd like to further clarify how this software goes beyond offering a "single functionality". The scope review should take about 1 week to complete and we will get back to you shortly here.

Kevin-Mattheus-Moerman commented 2 years ago

@editorialbot check references

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

OK DOIs

- None

MISSING DOIs

- 10.1039/b926434e may be a valid DOI for title: Parameter-free calculations of X-ray spectra with FEFF9
- 10.1103/physrevb.83.115106 may be a valid DOI for title: Bethe-Salpeter equation calculations of core excitation spectra
- 10.1088/2516-1075/ab3123 may be a valid DOI for title: Bethe-Salpeter equation for absorption and scattering spectroscopy: implementation in the exciting code

INVALID DOIs

- None
matthewcarbone commented 2 years ago

@Kevin-Mattheus-Moerman sure no problem. Yep I read the submission guidelines before submitting and I noticed that we are slightly below the 1000 line standard requirement (CodeCov at least says 912) and thought we should take our chances since we're well above the 300 line desk-reject limit. I am happy to provide additional justification of why this belongs in JOSS.

From studying your repository and paper it looks to me like this work is a "generator of input files", as such this software perhaps classifies as a "single functionality" package.

While it is true that Lightshow is designed for writing computational spectroscopy input files, I want to highlight a few features that put this outside of the "single functionality" box, at least in my opinion.

  1. If we only wrote input files for one type of code and did not offer a uniform abstraction for easy extendability, then I would agree it's probably too simple. However, Lightshow currently can write input files for five different types of x-ray spectroscopy softwares (as noted in the manuscript, these are FEFF, VASP, EXCITING, XSPECTRA and OCEAN). It can (and does) also do other things, which I detail later.
  2. We also handle an annoying "atom alignment" problem for those users who wish to do e.g. benchmarking. In other words, despite the fact that some codes are "cluster-based" and some are "supercell-based" (and different codes order atoms differently in their input files) every input file for material X and atom N for FEFF calculations will be the "same atom" in every other type of calculation. This is extensively tested and I am not aware of another software that unifies this process, which is key for generating data for benchmarking or data-driven purposes when multiple "levels of theory" are required.

So I suppose that our code offers at least two key functionalities. Depending on how you define functionalities, I think I could also argue (as I will later down below) that we have at least five key functionalities given Lightshow has the ability to write input files for five different simulation codes, and each comes with its own unique challenges.

You are correct that the current scope of this code is to write computational spectroscopy input files, but it can do this in many ways depending on what users want to do. For example, key functionality is exposed for every one of the types of codes we write input files for that allow users to tune code-specific (VASP potential files) and code-agnostic properties (the number of bands in a supercell calculation). Users can decide to pull materials data from the Materials Project, or to load data manually from their own hard drive. Etc.

Additionally, the code does (in my opinion, clearly) meet all of the other requirements, including feature completeness, packaged according to community, etc. Importantly, we are also designed to be extendable. Lightshow does not just need to write computational spectroscopy input files. In fact, for users' convenience in calculating the appropriate energy shifts between a neutral and core-hole calculation, we make it easy to run ground state convergence calculations (non-spectroscopy) in VASP, and some of the other codes. If this is too domain-specific an explanation, think "we also do other non-spectroscopy calculations that users might need in their analysis of the spectroscopy results".

Finally, I would ask that the editors consider that significant effort went into "golfing the code" down from bloated to what it is now. The implementation of a uniform abstraction for writing the input files saved us considerable space. So while the code is ~900 lines, it is a dense 900 lines that started from many more. We could've left it as-is and it probably still would've worked fine, but the team decided it would be worth the effort to consolidate since that would make the software much more maintainable.

It does look like it is quite a technical challenge that is being solved, as generating these files sounds non-trivial.

Thank you, indeed it definitely was.

Point 1 above for especially VASP, EXCITING, XSPECTRA and OCEAN was a huge challenge to get right. We actually have core developers from EXCITING and OCEAN on this team (Christian, Claudia and John), and experts on the other three codes, to ensure that we did.

Point 2 ("atom alignment") was another very annoying problem we needed to solve robustly in order to benchmark these codes against each other and generate machine learning databases for ongoing work. Due to the nuances of each individual code this also proved to be very challenging. It's also a pretty easy pitfall to get stuck in (speaking from experience here), which we abstract away for the user.

The scope review should take about 1 week to complete and we will get back to you shortly here.

Thank you for your time. I would only ask on behalf of our team that the editors consider the above. I do believe that Lightshow is multifunctional, and it was designed for extendability so that more functionalities can be easily added by the community. Please do feel free to ask any questions should you have them.

Kevin-Mattheus-Moerman commented 2 years ago

@x94carbone we have reviewed your work and your comments and feel it may be in scope for JOSS. I have just removed the query-scope label. However I may need to add the waitlisted label now since the editors in this domain are currently handling many other submissions. We will assign an editor once one becomes available.

matthewcarbone commented 2 years ago

@Kevin-Mattheus-Moerman ahh fantastic, thank you, very happy to hear we're still under consideration! No worries about the waitlist, if nothing else I understand being busy 😁

ppxasjsm commented 2 years ago

@editorialbot assign @ppxasjsm as editor

editorialbot commented 2 years ago

Assigned! @ppxasjsm is now the editor

ppxasjsm commented 2 years ago

@Kevin-Mattheus-Moerman I am happy to edit this!

ppxasjsm commented 2 years ago

@x94carbone, I'll take this paper on as an editor. I do have some other work on my plate right now, but I'll do my best to process this speedily!

If you have any suggestions for potential reviewers then please mention them here in this thread (without tagging them with an @). In addition, this list of people have already agreed to review for JOSS and may be suitable for this submission (please start at the bottom of the list). This will speed up the process for me to identify suitable reviewers.

matthewcarbone commented 2 years ago

@ppxasjsm fantastic, thank you! No worries about any delays and whatnot, we're all in the same boat in terms of being busy, I'd think.

Regarding reviewers, sure I'll take a look at that list as soon as I can and will get back to you πŸ‘

Kevin-Mattheus-Moerman commented 2 years ago

@editorialbot assign @ppxasjsm as editor

editorialbot commented 2 years ago

Assigned! @ppxasjsm is now the editor

Kevin-Mattheus-Moerman commented 2 years ago

Sorry @ppxasjsm I missed that you already assigned yourself. Thanks

matthewcarbone commented 2 years ago

@ppxasjsm ok some possible reviewers:

I believe the first two would be the best fits, probably could start there!

matthewcarbone commented 2 years ago

@ppxasjsm Just making sure you saw the list above. Let me know if there's anything else I can do!

ppxasjsm commented 1 year ago

@maurov, I was wondering if you'd be willing to review this paper for me?

ppxasjsm commented 1 year ago

@marwanalam2, You seem to have the right expertise to review this paper, would you be available for this?

ppxasjsm commented 1 year ago

@ppxasjsm Just making sure you saw the list above. Let me know if there's anything else I can do!

No sorry, I had indeed missed it somehow and then spent a week drowning in many urgent tasks, apologies for the slow pick up.

maurov commented 1 year ago

@maurov, I was wondering if you'd be willing to review this paper for me?

@ppxasjsm yes, this paper fits my domain of expertise and I am happy to review it. Please, let me know the steps to follow, as this is my first review for JOSS.

ppxasjsm commented 1 year ago

Brilliant, thank you @maurov. For now, I'll add you as a reviewer. I am still looking for a second reviewer, so it will take a little moment before more reviewing instructions will follow. In the meantime you can have a look here.

ppxasjsm commented 1 year ago

@editorialbot add @maurov as reviewer

editorialbot commented 1 year ago

@maurov added to the reviewers list!

matthewcarbone commented 1 year ago

@ppxasjsm just a quick FYI I changed my GH username! Probably should be updated in the original post πŸ‘

matthewcarbone commented 1 year ago

@ppxasjsm @maurov Just following up on the review process! Is there anything else I can do? Also, I believe we're actively under review now, but the tag still says pre-review. Is this still correct? Thanks!

maurov commented 1 year ago

@matthewcarbone I did a quick evaluation beginning of December and then I put this task on hold, waiting @ppxasjsm for starting the review process. Please, let me know if I should start the review or wait until a second reviewer is found.

matthewcarbone commented 1 year ago

@maurov got it, thank you! As you say, I suppose we need @ppxasjsm to give some guidance on the matter.

matthewcarbone commented 1 year ago

@ppxasjsm any updates on this? Thanks!

ppxasjsm commented 1 year ago

I am sorry for being slow, I lost track of this over the holidays and the beginning of the new year. I am still waiting for a second reviewer, but we can start the review in any case and I'll add the second reviewer later on.

ppxasjsm commented 1 year ago

@editorialbot start review

editorialbot commented 1 year ago

OK, I've started the review over in https://github.com/openjournals/joss-reviews/issues/5182.