search

openjournals / joss-reviews

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

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

Closed editorialbot closed 1 year ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@matthewcarbone<!--end-author-handle-- (Matthew Carbone) Repository: https://github.com/AI-multimodal/Lightshow Branch with paper.md (empty if default branch): joss-manuscript Version: v0.1.3 Editor: !--editor-->@ppxasjsm<!--end-editor-- Reviewers: @maurov, @larsenkg Archive: 10.5281/zenodo.8118592

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)

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

@maurov, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @ppxasjsm 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 ✨

Checklists

📝 Checklist for @maurov

📝 Checklist for @larsenkg

editorialbot commented 1 year ago

Hello humans, 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 1 year ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.09 s (454.9 files/s, 70133.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          18            679           1280           2573
reStructuredText                12            201            224            201
Markdown                         1             15              0            158
YAML                             3             10             15            126
TeX                              1             10              0             97
TOML                             1              6              1             82
Bourne Shell                     1             25              6             80
Jupyter Notebook                 1              0            431             44
DOS Batch                        1              8              1             27
make                             1              4              6             10
JSON                             1              0              0              1
-------------------------------------------------------------------------------
SUM:                            41            958           1964           3399
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 1200

editorialbot commented 1 year ago

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

editorialbot commented 1 year 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
ppxasjsm commented 1 year ago

@matthewcarbone could you please fix the missing DOIs please and generate the pdf again using the editorial bot?

matthewcarbone commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

matthewcarbone commented 1 year ago

@ppxasjsm I've added the DOIs explicitly for those manuscripts. Should I add every DOI? Seems like it's imbalanced now. Certain references have DOIs and some don't!

ppxasjsm commented 1 year ago

Yes please add the DOI if you don't mind.

maurov commented 1 year ago

Review checklist for @maurov

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

maurov commented 1 year ago

@matthewcarbone I have started the review and my main concern is that I do not understand why you have decided to create a new software from scratch instead contributing to existing well established open-source Python projects in the field, namely, Pymatgen and ASE?

Since you are closer to Pymatgen, why not simply extending pymatgen.io with OCEAN, EXCITING and Xspectra?

Furthermore, in the statement of need is written that Lightshow is a tool for XAS simulation and analysis. In the current status, I see only a (possibly redundant) tool for generating XAS input files, but I do not see any tool helpful for the analysis of the XAS simulations. As part of the XAS community myself, I would expect much more on the analysis side. For example: a tool for extracting local coordination environment from the input structures or modify the local environment (e.g. angles, coordination, distances); perform convergence tests versus a given structural parameter; handle the generated spectra (e.g. plots and validation/comparison with experimental data).

I think that all this should be clearly stated out in the software paper and in the documentation.

The current documentation is minimalist and does not show a single case of the full workflow: from the input structure down to a XAS spectrum. I think this should go into the quickstart or refer to a more complete notebook (just generating the input file is not enough). In my opinion, the first/minimal thing a generic user would like to do with Lightshow is: 1) load a CIF file, 2) generate an input file for a given calculator, 3) run the calculator, 4) plot the XAS spectrum. Points 3 and 4 are very important in the workflow.

I do think there is a need in the scientific community for such a tool and your research group has the expertise for it, but with the current contribution you are sharing only the tip of the iceberg. So, please, add more detailed notebooks with real cases examples.

I would also describe in the documentation how to extend Lightshow with other calculators or add at least input file generation for FDMNES, which is well known in the XAS community.

These are my general comments. I will add more technical aspects later on or directly link to Github issues.

matthewcarbone commented 1 year ago

@maurov thanks for the initial feedback. I can try to address some of your concerns here, and I'll get in touch with the team to handle the more technical comments regarding e.g. notebooks.

I do not understand why you have decided to create a new software from scratch instead contributing to existing well established open-source Python projects in the field, namely, Pymatgen and ASE? Since you are closer to Pymatgen, why not simply extending pymatgen.io with OCEAN, EXCITING and Xspectra?

In a perfect world, this is the correct open-source solution, but unfortunately there are fundamental limitations we are bound by at my institution. For one, this work was funded by members of BNL and therefore we are tied down by certain requirements. For example, a full "project," if you will, is required to be licensed in a certain way. Making these contributions upstream to ase or pymatgen was unfortunately just not an option for us. That said, now that our code is published, it is certainly permissible for the maintainers of these packages (or honestly anyone, including us) to copy/paste Lightshow code upstream as long as long as they copy/paste the license with it!

Furthermore, in the statement of need is written that Lightshow is a tool for XAS simulation and analysis. In the current status, I see only a (possibly redundant) tool for generating XAS input files, but I do not see any tool helpful for the analysis of the XAS simulations.

I don't think it's redundant. It is slightly redundant for FEFF but we've not found a tool that does what we want for the other codes (Pymatgen can handle VASP to some extent but it's very confusing, at least to me). For benchmarking specifically we offer some functionality that is uniquely useful (matching unique sites between different codes). That said, analysis tools are absolutely the next step and are definitely going to be a very useful addition for us and others.

As part of the XAS community myself, I would expect much more on the analysis side. For example: a tool for extracting local coordination environment from the input structures or modify the local environment (e.g. angles, coordination, distances); perform convergence tests versus a given structural parameter; handle the generated spectra (e.g. plots and validation/comparison with experimental data).

All great ideas that I agree with. We just felt it was out-of-scope for the "v1" release.

In my opinion, the first/minimal thing a generic user would like to do with Lightshow is: 1) load a CIF file, 2) generate an input file for a given calculator, 3) run the calculator, 4) plot the XAS spectrum. Points 3 and 4 are very important in the workflow.

At its current stage, Lightshow is not a workflow code. The subtleties in developing workflows for e.g. different computer architectures was just not something we wanted to address right now. Even ase often just requires an executable in the PATH, and then just calls that executable from a lightweight Python function. We figured at this stage that part was not really required for what we want to do.

Lightshow's intention at this stage is to write the input files for the user. Running the code is a whole different can of worms that we agreed was not in scope for the first release. That said, I think one thing we can do at least is to make a tutorial of how you'd run the codes using a simple bash script on some HPC, or something like this.

I do think there is a need in the scientific community for such a tool and your research group has the expertise for it, but with the current contribution you are sharing only the tip of the iceberg. So, please, add more detailed notebooks with real cases examples.

Will do!

I would also describe in the documentation how to extend Lightshow with other calculators or add at least input file generation for FDMNES, which is well known in the XAS community.

FDMNES is actually at the top of our list for the next kind of code to add. We definitely are seeing it all over the place and agree it makes sense to add to Lightshow.

Kevin-Mattheus-Moerman commented 1 year ago

@ppxasjsm thanks for your help editing this submission. Can you check in and see where this review is at? Perhaps the reviewers could use a reminder.

ppxasjsm commented 1 year ago

@maurov Just following up on @Kevin-Mattheus-Moerman suggestion and @matthewcarbone's responses to your initial feedback. Do you think you will be able to proceed with the review, or are there too many substantial concerns?

maurov commented 1 year ago

@ppxasjsm @Kevin-Mattheus-Moerman thank you very much indeed for the reminder, but I was waiting some more activity on the repository after @matthewcarbone reply to my comments. In particular, I was expecting more notebooks with real cases examples and a tutorial showing how to use the code in the whole workflow. Once this is done, the paper could be accepted in my opinion. There is a need in the community and Lightshow goes in the right direction. I was expecting more for a first release, but I understand authors' concerns too. Anyway, I will complete the review soon.

matthewcarbone commented 1 year ago

Yup sorry for the delays. It's proposal writing season right now and things are really busy. However, my team and I are working on these notebooks and will get the merged in as soon as possible. Thanks, all!

ppxasjsm commented 1 year ago

Thanks for the update @matthewcarbone!

ppxasjsm commented 1 year ago

@editorialbot add @larsenkg to reviewers

editorialbot commented 1 year ago

@larsenkg added to the reviewers list!

ppxasjsm commented 1 year ago

@larsenkg, I think you said you'd be back from holidays about now? You can get started by generating your reviewer checklist using editorialbot with: @editorialbot generate my checklist

Please let me know if you have an issues.

larsenkg commented 1 year ago

Review checklist for @larsenkg

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

maurov commented 1 year ago

@matthewcarbone regarding the software paper, I think overall reads well. I recommend adding a couple of sentences about the fact that this code is part of a (much) larger workflow consisting in running the ab-initio codes, generating the simulated spectra and comparing/benchmarking with experimental spectra.

Please, could you consider the following points ?

matthewcarbone commented 1 year ago

@maurov thanks for the additional feedback! We'll update the manuscript with your suggestions. In addition, the tutorial notebook is now much more comprehensive. Here is the link to the commit, but it's currently on master.

maurov commented 1 year ago

@matthewcarbone thanks for updating the basic usage notebook with some more explicit parameters, even if going into the details of each code is beyond the scope.

One thing that is missing in the documentation is how one could contribute for adding support for additional calculators.

And I have also a couple of minor comments for the development installation:

larsenkg commented 1 year ago

@matthewcarbone I do not have any major concerns about this software or its suitability to JOSS. I am not in the XAS field, so some of the details of this software and its use cases are beyond me. The addition of the tutorial notebook was helpful. From what I do understand, this software will be a boon to your community. I only have a few thoughts relating to community guidelines and your references (and then some observations):

My observations on installing for development and running tests:

If you would like me to file an issue for either of these, let me know. 

install_doc_requirements_only () {
    python3 -c 'import toml; c = toml.load("pyproject.toml"); print(" ".join(c["project"]["optional-dependencies"]["doc"]))' | xargs pip install 
}

install_test_requirements_only () {
    python3 -c 'import toml; c = toml.load("pyproject.toml"); print(" ".join(c["project"]["optional-dependencies"]["test"]))' | xargs pip install 
}

install_requirements() {
    python3 -c 'import toml; c = toml.load("pyproject.toml"); print(" ".join(c["project"]["dependencies"]))' | xargs pip install 
}
matthewcarbone commented 1 year ago

If you would like me to file an issue for either of these, let me know.

@larsenkg that would be wonderful (though not necessary)! Thank you for the review! 😁

ppxasjsm commented 1 year ago

@maurov Do I see this correctly, that you are happy with everything and you have completed your part of the review?

maurov commented 1 year ago

@ppxasjsm I have completed my part of the review. I am waiting for @matthewcarbone to implementing the minor corrections to the software paper and few additions to the documentation (mainly contributing guidelines), as I have previously proposed. Once this is done, feel free to move on with the review process.

ppxasjsm commented 1 year ago

@matthewcarbone please let m know when you think you have addressed these small issues.

matthewcarbone commented 1 year ago

@ppxasjsm yup you got it. We're actually preparing a lot of material right now for a workshop, so once that's done I can merge those efforts into the review process here.

ppxasjsm commented 1 year ago

HI @matthewcarbone, just doing a quick check-in, in terms of the new material to see how you are getting on there. If you could let me know a rough timeline when you think you'll be able to merge this by I will only check-in after the suggested timeline. Thanks, Toni

ppxasjsm commented 1 year ago

👋 @matthewcarbone just doing another check-in here. How are you getting on? Do you have an idea when you will have implemented the reviewer's suggestions? Anything I can help with?

matthewcarbone commented 1 year ago

@ppxasjsm Hi Toni, apologies for all the delays. It's another proposal-writing cycle and I've just been tied up with other things. I will go through the raised issues and try to gather up what needs to be done. Then I can try to provide a timeline 👍

matthewcarbone commented 1 year ago

Ok so my required todo checklist as it stands:

Other possible todos:

ppxasjsm commented 1 year ago

There is no particular rush, I just wanted to periodically check in that is all. Thank you for the update and I can now just check your to do list. If you can give me a timeline great, otherwise I'll check in again in a month, unless you are done sooner.

matthewcarbone commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

matthewcarbone commented 1 year ago

Good bot.

@ppxasjsm I think I can probably get most of this done in the next week or so. Only a few more things to do and they're basically all documentation related. Thanks!

matthewcarbone commented 1 year ago

@ppxasjsm ok all done!

@larsenkg since it was your idea, do you want to open a PR to fix the Windows-related install? 👍

ppxasjsm commented 1 year ago

Thanks @matthewcarbone!

ppxasjsm commented 1 year ago

I have gone through the paper. Here is a small list of corrections from me:

Can you please address these, double check you are happy with everything and then we can proceed to recommend accept. For this step you will need to do the following things:

matthewcarbone commented 1 year ago

@ppxasjsm Sure on it. One question though. I'm not sure I understand this request:

Finally, we also note that Lightshow is ... use Lightshow here too!

Can you clarify what you'd like me to change?

ppxasjsm commented 1 year ago

There is one part in your Markdown where you don't use the formatting of Lightshow as a code part but just as a word. To me that looked inconsistent. If you disagree that is fine.

matthewcarbone commented 1 year ago

Keeping my own copy so I can keep track of progress:

I have gone through the paper. Here is a small list of corrections from me:

  • [x] Can you make sure that X-ray is written as X-ray everywhere?
  • [x] Frist-principles should be hyphenated consistently.
  • [x] The emergence of the high-performance computing hardware architecture-> The emergence of high-performance computing hardware architecture
  • [x] continue to fuel the advance of first-principles simulations in materials design. -> continues to fuel the advances of first-principles simulations in materials design.
  • [x] ultraviolet–visible uses the wrong dash I believe. Please double check.
  • [x] Finally, we also note that Lightshow is ... use Lightshow here too!

Can you please address these, double check you are happy with everything and then we can proceed to recommend accept. For this step you will need to do the following things:

  • [x] Make a tagged release of your software, and list the version tag of the archived version here.
  • [x] Archive the reviewed software in Zenodo or a similar service (e.g., figshare, an institutional repository)
  • [x] Check the archival deposit (e.g., in Zenodo) has the correct metadata. This includes the title (should match the paper title) and author list (make sure the list is correct and people who only made a small fix are not on it). You may also add the authors' ORCID.
  • [x] Please list the DOI of the archived version here.
matthewcarbone commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

matthewcarbone commented 1 year ago

DOI on Zenodo: https://doi.org/10.5281/zenodo.8118592

matthewcarbone commented 1 year ago

@ppxasjsm all done. Note for UV-vis apparently JOSS doesn't render the triple dash correctly. I think it's fine as is!

ppxasjsm commented 1 year ago

@editorialbot check references