search

openjournals / joss-reviews

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

[REVIEW]: pyCADMium: Chemical Atoms in Diatomic Molecules. A prolate spheroidal Python module for embedding calculations. #4459

Closed editorialbot closed 2 years ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@VHchavez<!--end-author-handle-- (Victor Hugo Gonzalez Chavez) Repository: https://github.com/wasserman-group/pyCADMium Branch with paper.md (empty if default branch): main Version: v0.9.1 Editor: !--editor-->@lucydot<!--end-editor-- Reviewers: @raghurama123, @srmnitc Archive: 10.5281/zenodo.7039713

Status

status

Status badge code:

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

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

@raghurama123 & @srmnitc, 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 @lucydot 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 @srmnitc

📝 Checklist for @raghurama123

editorialbot commented 2 years 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 2 years ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.26 s (636.8 files/s, 140592.3 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
JavaScript                      14           2517           2692           9887
Python                          86           1946           2397           5451
HTML                            24           1702              3           4544
SVG                              1              0              0           2671
CSS                              7            276             85           1317
Markdown                        10             91              0            281
TeX                              1             15              4            165
reStructuredText                15            252            192            130
YAML                             6             16             21            120
Bourne Shell                     1              5              8             28
DOS Batch                        1              8              1             27
make                             1              4              6             10
-------------------------------------------------------------------------------
SUM:                           167           6832           5409          24631
-------------------------------------------------------------------------------

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

Wordcount for paper.md is 842

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

OK DOIs

- 10.18257/raccefyn.960 is OK
- 10.1103/physreva.82.024501 is OK
- 10.1021/jp504058s is OK
- 10.1021/acs.jctc.6b01050 is OK
- 10.1063/1.5051455 is OK
- 10.1002/qua.25425 is OK
- 10.1002/wcms.1617 is OK
- 10.1126/science.1158722 is OK
- 10.1103/physrevlett.91.146401 is OK
- 10.1016/j.cpc.2012.05.007 is OK
- 10.1016/c2009-1-28537-6 is OK
- 10.1002/qua.24355 is OK
- 10.1039/C5CP00351B is OK
- 10.1063/1.5003825 is OK
- 10.1063/1.442958 is OK

MISSING DOIs

- None

INVALID DOIs

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

srmnitc commented 2 years ago

Review checklist for @srmnitc

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

raghurama123 commented 2 years ago

Review checklist for @raghurama123

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

raghurama123 commented 2 years ago

Review:

I welcome the submission of the package pyCADMium to perform electronic structure calculations in prolate spheroidal coordinates. This is a useful contribution. However, there are several points in the Software paper that I think should be addressed by the authors for the submission to be accepted for publication in JoSS. Once these comments are addressed, I can review them again and check off the entries under Software paper.

The software's age, number of lines, commits, and author numbers comply with JoSS' guidelines. The entire package has been submitted by @VHchavez. The second and third authors have contributed to the methods, and appropriate references are cited in the paper. So far, the software has been cited in a thesis. I think it is very likely that the code will be cited by other researchers. I think the software can be applied to time-dependent problems through TDDFT simulation of electron dynamics.

The installation of the package and its documentation was simple. Along with libxc (I installed pylibxc2), the package also depends on pydantic that I have installed. This could be mentioned in the main README.md file

I have tested most of the example codes provided in a separate repository and the results are convincing.

Software paper:

  1. A major criticism of the paper is the lack of citations to the DARSEC code which also allows similar calculations. Some applications done using DARSEC are listed here.

However, it appears that DARSEC is not available on a public forum. So, the present submission, pyCADMium, provides the only open-source option. Therefore, I recommend accepting the submission in JOSS once my following comments are addressed.

However, the authors should cite the DARSEC code and state its availability or the lack of it. Independent of the programming language and libraries utilized, the authors must comment on the methodology implemented in pyCADMium and DARSEC. How similar are the underlying methodologies in the two codes?

  1. Here is a list of corrections and suggestions to improve the write-up.

Line-6. Unversity to University

Line-10, With only a handful of electrons, their computing time is short. This is a bit confusing. A reader may wonder if a diatomic with heavy atoms (say Uranium dimer) qualifies as a system with a handful of electrons. Please remove this sentence and/or include another point`

Line-45 Konh to Kohn

Lines-53/54 additve to additive

Line-63 availiable to available

Line-66 Insitute to Institute

Line-67 continious to continuous

I think it is easy to divide the Summary into two parts: Introduction and Summary. Here is a suggested modification that also fixes some typos.

Introduction

Diatomic molecules are among the most useful systems to test new ideas in quantum chemistry:

Most practical calculations use a finite basis set of fixed functional forms to represent operators and quantities like potentials, orbitals, and densities [@12SO;@13H]. Such approaches require analytic expressions for the matrix representation of various operators and often exhibit poor basis set convergence. The basis set incompleteness error can be minimized by increasing the number of basis functions used, but it cannot be entirely eliminated in practice because of the lack of parameters (for instance, exponents, contraction coefficients, etc.) for large basis sets. On the other hand, grid-based methods intrinsically allow for an accurate representation of operators and permit the use of arbitrarily large basis sets [@octopus]. Nevertheless, the number of points required to achieve a significant accuracy can quickly become unmanageable. pyCADMium presented in this work, uses a prolate spheroidal (PS) grid to circumvent these issues for atoms and diatomic molecules.

Summary

In this work, we introduce pyCADMium, a Python module that uses a PS coordinate grid to accurately perform computational chemistry calculations on systems with cylindrical symmetry. The name is an acronym for Chemical Atoms in Diatomic Molecules".pyCADMium`` originated in a proprietary programming language but has been rewritten from the ground up as an open-source alternative. The code has been the main driving force behind the development of "Partition Density Functional Theory," [@20CW;@10EW;@17NW;@18JW] a method that uses quantum embedding to lower the cost of a calculation and fixes problems inherent to density-functional approximations [@08CY].
The PS grid is a coordinate system formed by revolving an elliptic coordinate plane around the line intersecting the two foci. These planes are formed by ellipses and hyperbolae that share the same focus [@arfken]. Atoms and diatomics are ideally suited for this coordinate system given that each focus can be used to allocate an atom. Additionally, the PS grid is denser around the foci in its cartesian representation, where we expect wave functions of molecular systems to change rapidly [@17RS].

Some corrections in the README.md file

This is not technically true as the grids form the basis set now, and each basis function is a Dirac-delta function. Instead, a suggestion is to use "- Calculations of atoms and diatomic molecules in real space"`

Another point can be changed to

On another point, semianalitically to semi-analytically

raghurama123 commented 2 years ago

I have provided my review. I am very positive about the utility of the package. I think it is a very useful tool. As stated, the write-up requires some edits. Once these are done, I will check off the remaining boxes.

VHchavez commented 2 years ago

Hello @raghurama123, thank you so much for your review!! :) I am in the middle of a few hectic days, but I'll address your comments in just a few days.

srmnitc commented 2 years ago

@lucydot @VHchavez I have completed my first review of the package. I think pyCADMium is a useful package that should be published in JOSS. The code is well-written, and the set of examples are convincing. The JOSS paper is also well written. There are some issues which prevent me from checking all the boxes. Most of them are related to documentation and functionality. I have raised two separate issues which provide a detailed description of the issues (https://github.com/wasserman-group/pyCADMium/issues/10, https://github.com/wasserman-group/pyCADMium_examples/issues/3).

Functionality: There are a number of useful examples. However, the dependencies for running these examples are not correctly specified and some of them run into errors. Also, including these examples as part of the documentation will greatly improve the utility of the documentation.

Documentation: Documentation is split in the README and the github pages. Github pages documentation is very well written, but could benefit from the addition of some of the things that I mentioned in the issue. The module comes with a full set of automated tests, documenting how to run them will make it much easier for the user.

Overall, the package is useful and deserve publication. I will be happy to check the remaining boxes when the issues are addressed. Thanks again for the nice work @VHchavez .

lucydot commented 2 years ago

Thank you very much @raghurama123 and @srmnitc for your thorough and thoughtful reviews.

@VHchavez - over to you! It seems you have some useful points to address, particularly with regard to documentation. If you are unable to act on / respond to the reviewer suggestions in the next couple of days, an approximate timeline for when this will be possible would be appreciated. I encourage progress updates (whether positive or if you've hit some roadblocks) to be made in this thread - you don't need to always go into detail, but just so we know what is ticking along. And feel free to ask me any questions here.

lucydot commented 2 years ago

/ooo Jun 31 until July 7

lucydot commented 2 years ago

Hi @VHchavez its been a couple of weeks - have you got any update on progress so far?

VHchavez commented 2 years ago

Thanks for the poke, @lucydot. Hopefully, I'll get the edits done by the end of the week.

lucydot commented 2 years ago

Excellent, please give us an update with whatever progress you make.

I am going away on leave tomorrow (15th Jul) for one week - my responses during this time will be delayed (perhaps until I return).

VHchavez commented 2 years ago

Currently working on the docs issues highlighted by @srmnitc.

lucydot commented 2 years ago

Hello @VHchavez a prompt again for any updates, or approximate timescales?

ps, I will be on holoiday for one week - back on 20th August

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

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

VHchavez commented 2 years ago

Hello Folks @lucydot @raghurama123 @srmnitc!

Thank you so much for being incredibly patient. I have addressed each reviewer's comments in each issue/PR. I believe I have addressed all of them according to your specifications. I have reread the proof, and I think I am satisfied with it. I hope you agree with the changes made.

I hugely appreciate your help with the project. Thanks again, VH

lucydot commented 2 years ago

@srmnitc @raghurama123 looks like it is back to you - @VHchavez has responded to the points raised over at the issue - https://github.com/wasserman-group/pyCADMium/issues/10.

raghurama123 commented 2 years ago

Hi all, I will respond in a couple of days.

raghurama123 commented 2 years ago

I am happy with the present version of the software paper. I would like to add that I have performed some calculations with pyCADMium and I was able to figure out how to make any modifications to the source. However, I have not explored all possible options in the code.

The authors have addressed all my previous remarks and included a citation to the DARSEC code. As a difference between the implementations in pyCADMium and DARSEC, the authors have clarified that pyCADMium uses libxc while DARSEC presumably uses XC functionals coded up separately. I could not find elsewhere if DARSEC also uses libxc. Since pyCADMium provides the source code, other researchers can play with the code and perform calculations in prolate spheroidal coordinates.

Here are a couple of small typos in the paper, which the authors may want to fix. line-43: Place one (A, ) does not make sense. Maybe this can be changed to Place one atom (A) at the origin? line-46: Remove period after defined

Once these typos are fixed, the article and the code may be accepted without any further review.

VHchavez commented 2 years ago

Thanks for your comments, @raghurama123 :)

In regards to the comments on line 43. The initial idea was to add a ∅ to signify that the site was empty, but I believe it gets lost in the process of generating the pdf. If I'm unable to render it appropriately, do you think (A, 0) would be clear enough? Otherwise, I can leave it as (A). I like the idea of having the two different sites explicit since the grid is generated independently of whether there's an atom or not.

Line 46 has been updated.

raghurama123 commented 2 years ago

Okay, that makes sense. I was confused about the notation. Origin shouldn't be the right choice in this coordinate system as the density of grids may not be good there. Perhaps you can change line 43 to Place one atom at a focus (A) ?

VHchavez commented 2 years ago

Sounds great. All your changes have been added @raghurama123.

srmnitc commented 2 years ago

@VHchavez Thank you for all the updates, from my side everything looks good. Not blocking acceptance, but I would strongly recommend updating the docstrings further for the code to be really helpful.

@lucydot All my concerns have been addressed, and I recommend the code and the associated paper for publication. I am hopeful that this tool will be useful for the community. Thanks to all the developers for the good work!

lucydot commented 2 years ago

That's excellent, thank you very much @raghurama123 and @srmnitc for your thoughtful reviews ✨

I will do a final read through of the paper, alongside some admin checks, before suggesting to the editors-in-chief that we go ahead with publication!

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

lucydot 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

- 10.18257/raccefyn.960 is OK
- 10.1103/physreva.82.024501 is OK
- 10.1021/jp504058s is OK
- 10.1021/acs.jctc.6b01050 is OK
- 10.1063/1.5051455 is OK
- 10.1002/qua.25425 is OK
- 10.1002/wcms.1617 is OK
- 10.1126/science.1158722 is OK
- 10.1103/physrevlett.91.146401 is OK
- 10.1016/j.cpc.2012.05.007 is OK
- 10.1016/c2009-1-28537-6 is OK
- 10.1002/qua.24355 is OK
- 10.1039/C5CP00351B is OK
- 10.1063/1.5003825 is OK
- 10.1063/1.442958 is OK

MISSING DOIs

- 10.1021/ct800485v may be a valid DOI for title: Fully numerical all-electron solutions of the optimized effective potential equation for diatomic molecules
- 10.1063/1.1535422 may be a valid DOI for title: A direct optimization method for calculating density functionals and exchange–correlation potentials from electron densities

INVALID DOIs

- None
lucydot commented 2 years ago

Hi @VHchavez could you add the missing DOIs to your paper, as suggested by editorialbot above?

lucydot commented 2 years ago

Once you have done added the missing DOIs (see message above), please:

Report the archive DOI and version number in this thread 👇

VHchavez 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

- 10.18257/raccefyn.960 is OK
- 10.1103/physreva.82.024501 is OK
- 10.1021/jp504058s is OK
- 10.1021/acs.jctc.6b01050 is OK
- 10.1063/1.5051455 is OK
- 10.1002/qua.25425 is OK
- 10.1002/wcms.1617 is OK
- 10.1126/science.1158722 is OK
- 10.1103/physrevlett.91.146401 is OK
- 10.1016/j.cpc.2012.05.007 is OK
- 10.1021/ct800485v is OK
- 10.1063/1.1535422 is OK
- 10.1016/c2009-1-28537-6 is OK
- 10.1002/qua.24355 is OK
- 10.1039/C5CP00351B is OK
- 10.1063/1.5003825 is OK
- 10.1063/1.442958 is OK

MISSING DOIs

- None

INVALID DOIs

- None
VHchavez commented 2 years ago

@lucydot Hello Lucy, the project with the tag v0.9.1 has been uploaded to Zenodo with the DOI: 10.5281/zenodo.7039713

:)

lucydot 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

- 10.18257/raccefyn.960 is OK
- 10.1103/physreva.82.024501 is OK
- 10.1021/jp504058s is OK
- 10.1021/acs.jctc.6b01050 is OK
- 10.1063/1.5051455 is OK
- 10.1002/qua.25425 is OK
- 10.1002/wcms.1617 is OK
- 10.1126/science.1158722 is OK
- 10.1103/physrevlett.91.146401 is OK
- 10.1016/j.cpc.2012.05.007 is OK
- 10.1016/c2009-1-28537-6 is OK
- 10.1002/qua.24355 is OK
- 10.1039/C5CP00351B is OK
- 10.1063/1.5003825 is OK
- 10.1063/1.442958 is OK

MISSING DOIs

- Errored finding suggestions for "Fully numerical all-electron solutions of the opti...", please try later
- 10.1063/1.1535422 may be a valid DOI for title: A direct optimization method for calculating density functionals and exchange–correlation potentials from electron densities
- Errored finding suggestions for "Modern quantum chemistry: introduction to advanced...", please try later

INVALID DOIs

- None
lucydot 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:

lucydot commented 2 years ago

@VHchavez - the latest paper doesn't include the changes you made - it looks like your commit which included the additional dois has been reverted? (https://github.com/wasserman-group/pyCADMium/commit/26b4751229e412049f4f968ab3d475ff6fd0de9d)

VHchavez commented 2 years ago

@lucydot reverted.

lucydot commented 2 years ago

@editorialbot generate pdf

lucydot 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

- 10.18257/raccefyn.960 is OK
- 10.1103/physreva.82.024501 is OK
- 10.1021/jp504058s is OK
- 10.1021/acs.jctc.6b01050 is OK
- 10.1063/1.5051455 is OK
- 10.1002/qua.25425 is OK
- 10.1002/wcms.1617 is OK
- 10.1126/science.1158722 is OK
- 10.1103/physrevlett.91.146401 is OK
- 10.1016/j.cpc.2012.05.007 is OK
- 10.1021/ct800485v is OK
- 10.1063/1.1535422 is OK
- 10.1016/c2009-1-28537-6 is OK
- 10.1002/qua.24355 is OK
- 10.1039/C5CP00351B is OK
- 10.1063/1.5003825 is OK
- 10.1063/1.442958 is OK

MISSING DOIs

- Errored finding suggestions for "Modern quantum chemistry: introduction to advanced...", please try later

INVALID DOIs

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