openjournals / joss-reviews

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

[REVIEW]: QuantNBody: a python package for quantum chemistry and physics to build and manipulate many-body operators and wave functions. #4759

Closed editorialbot closed 1 year ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@SYalouz<!--end-author-handle-- (SAAD YALOUZ) Repository: https://github.com/SYalouz/QuantNBody Branch with paper.md (empty if default branch): Version: v 1.0.0 Editor: !--editor-->@jarvist<!--end-editor-- Reviewers: @wcwitt, @erikkjellgren Archive: 10.5281/zenodo.7440993

Status

status

Status badge code:

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

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

@wcwitt, 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 @jarvist 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 @wcwitt

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

OK DOIs

- 10.1021/acs.jctc.7b00174 is OK
- 10.1063/5.0006074 is OK
- 10.21105/joss.00819 is OK
- 10.21468/scipostphys.2.1.003 is OK
- 10.21468/SciPostPhys.7.2.020 is OK
- 10.1088/2058-9565/ab8ebc is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1016/j.cpc.2012.02.021 is OK
- 10.1016/j.cpc.2012.11.019 is OK
- 10.22331/q-2020-10-11-341 is OK

MISSING DOIs

- None

INVALID DOIs

- None
SYalouz commented 1 year ago

Dear @jarvist, @wcwitt and @erikkjellgren,

For your information, I just updated the article with a brand new reference from our group mentioning the use of the QuantNBody package.

Also, as a quick reminder: all corrections and suggestions from @wcwitt and @erikkjellgren have been implemented. We look forward to hearing your feedback on them!

Best regards

erikkjellgren commented 1 year ago

Hi @SYalouz

Thank you for addressing all the points so nicely. Here a few issues/suggestions I have left.

Automated tests: It is very nice there is an automated test setup now. A nice feature from a users perspective would be a meassurement of well tested to code is, this could be by lines of code hit by the tests, see tools like coveralls.io. (This is just a nice to-have suggestion).

Community guidelines: I think the community guidelines should also be in the documentation. Some users might never look at the README.md if they seek information.

Quality of writing: Line 35: Openfermion should be OpenFermion

Documentation: There seem to be a lot of types and that sort of stuff in the documentation. Tho, it is not so bad that it makes the documentation confusing. I am sure the documentation can just be cleaned up later on, and should not be a problem for this JOSS publication.

SYalouz commented 1 year ago

Dear @erikkjellgren,

Thanks a lot for your very quick reply, we are happy that the modifications we made satisfy you !

Following you last remarks, I have just updated the documentations to include the community guidelines. I also corrected the typo about OpenFermion that you noticed in the paper.

We thank you also for your good advices concerning the automated tests and the documentations. We keep that in mind for future modifications.

We now wait for the feedback from @wcwitt !

Kind regards,

Saad

SYalouz commented 1 year ago

I will regenerate the paper with the latest correction suggested by Erik.

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

SYalouz commented 1 year ago

Dear @jarvist, Dear @wcwitt,

We implemented all the corrections required. We thus closed the issues raised by @wcwitt.

wcwitt commented 1 year ago

Look like good progress. I won't be able to take another detailed look until the week of 5-9 December.

SYalouz commented 1 year ago

Dear @wcwitt (and @jarvist)

Thanks for the message.

As we reach the end of the period you mentioned (5-9 December), I come back to you to know how is the review going on your side ?

wcwitt commented 1 year ago

I've made another round of review, which involved:

Overall things look good, and I think you're nearly there.

The Demo_FCI_ringFermiHubbard.py could not complete within 30 minutes on my laptop, although it finished very quickly when I reduced the number of active electrons and molecular orbitals from 8 to 6. Is this expected?

SYalouz commented 1 year ago

Dear @wcwitt,

Thanks for this remark concerning the example Demo_FCI_ringFermiHubbard.py We forgot to update the size of the system, indeed 8 electron in 8 orbital is already a lot and the Scipy eigensolver will take time if we want to access the full spectrum here.

I corrected this by reducing the size of the system to 6 electrons in 6 orbitals. Now it should run in a few seconds.

wcwitt commented 1 year ago

I raised that point partly because I remembered this line from the paper

As a result, QuantNBody allows the realization of larger scale calculations (up to 8 electrons in 8 spatial molecular orbitals in a few seconds).

Can you reconcile the two?

SYalouz commented 1 year ago

Dear @wcwitt,

I see your point. I corrected the code to use a sparse eigensolver from scipy. Now you can produce the results for 8 electrons in 8 MOs in a few seconds.

wcwitt commented 1 year ago

Great, runs for me now!

@jarvist, my checklist is finished and my concerns are addressed.

erikkjellgren commented 1 year ago

@jarvist, my checklist is finished and my concerns are addressed.

SYalouz commented 1 year ago

Dear Editor @jarvist,

Both referees @erikkjellgren and @wcwitt are satisfied with the corrections we made.

We are now waiting for your final feedback.

Kind regards

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

jarvist commented 1 year ago

Looking good! Have you made a DOI archive for the package yet @SYalouz ? I need to set this before sending it further.

SYalouz commented 1 year ago

Hi @jarvist !

Do you mean something like a zenodo DOI ?

jarvist commented 1 year ago

Yes! A software archive (with DOI) that will act as a permanent source of the software as reviewed by JOSS. I find the easiest way is to generate a release within GitHub & then have Zenodo mint a DOI + archive from that. But we can also accept uploading a Zip file to figshare or similar.

jarvist commented 1 year ago

I'm otherwise happy to recommend acceptance - I think your paper is well written and thorough, and I congratulate and thank both you and the reviewers for engaging so positively with the review process and working to improve both the manuscript and the software. Chapeau!

SYalouz commented 1 year ago

Dear @jarvist,

I have created a release and you have here the link to zenodo :

https://zenodo.org/record/7440993#.Y5rywezMJf0

jarvist commented 1 year ago

@editorialbot set 10.5281/zenodo.7440993 as archive

editorialbot commented 1 year ago

Done! Archive is now 10.5281/zenodo.7440993

jarvist commented 1 year ago

@editorialbot recommend-accept

editorialbot commented 1 year ago
Attempting dry run of processing paper acceptance...
editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1021/acs.jctc.7b00174 is OK
- 10.1063/5.0006074 is OK
- 10.21105/joss.00819 is OK
- 10.21468/scipostphys.2.1.003 is OK
- 10.21468/SciPostPhys.7.2.020 is OK
- 10.1088/2058-9565/ab8ebc is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1016/j.cpc.2012.02.021 is OK
- 10.1016/j.cpc.2012.11.019 is OK
- 10.22331/q-2020-10-11-341 is OK

MISSING DOIs

- 10.1063/5.0125683 may be a valid DOI for title: Quantum embedding of multi-orbital fragments using the Block-Householder-transformation

INVALID DOIs

- None
editorialbot commented 1 year ago

:warning: Error preparing paper acceptance. The generated XML metadata file is invalid.

ID figU003Aexample already defined
jarvist commented 1 year ago

Oh dear! That's one of the Guru meditations that might require an editorialbot developer to fix. Previously something similar was triggered by an error in the BibTex. I also see there's supposedly a missing DOI. Could you have a first look @SYalouz ?

SYalouz commented 1 year ago

I'll check it out right away.

SYalouz commented 1 year ago

Dear @jarvist ,

I just updated the reference of the article ;) It was an arxiv reference, but in the meantime the article has been accepted and published in a journal.

jarvist commented 1 year ago

@editorialbot recommend-accept

OK! Let's see if that fixes it...

editorialbot commented 1 year ago
Attempting dry run of processing paper acceptance...
editorialbot commented 1 year ago

:warning: Error preparing paper acceptance. The generated XML metadata file is invalid.

ID figU003Aexample already defined
editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1021/acs.jctc.7b00174 is OK
- 10.1063/5.0006074 is OK
- 10.21105/joss.00819 is OK
- 10.21468/scipostphys.2.1.003 is OK
- 10.21468/SciPostPhys.7.2.020 is OK
- 10.1088/2058-9565/ab8ebc is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1016/j.cpc.2012.02.021 is OK
- 10.1016/j.cpc.2012.11.019 is OK
- 10.22331/q-2020-10-11-341 is OK

MISSING DOIs

- 10.1063/5.0125683 may be a valid DOI for title: Quantum embedding of multi-orbital fragments using the block-Householder transformation

INVALID DOIs

- None
jarvist commented 1 year ago

That might have been a bit fast for your changes to sync. Also, the error is referring to a figure. I wonder if it's double defined in your .TeX somehow.

SYalouz commented 1 year ago

Sorry, I am going to check the figure :/

jarvist commented 1 year ago

Ah! You've given them both the same label name: \label{fig:example}. I think this should be unique.

SYalouz commented 1 year ago

Dear @jarvist,

Yes, and actually I didn't use it in my text as I directly wrote figure and figure 2... Anyway, I have just erased this. I hope it's going to work now.

SYalouz commented 1 year ago

@editorialbot check references

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

OK DOIs

- 10.1021/acs.jctc.7b00174 is OK
- 10.1063/5.0006074 is OK
- 10.21105/joss.00819 is OK
- 10.21468/scipostphys.2.1.003 is OK
- 10.21468/SciPostPhys.7.2.020 is OK
- 10.1088/2058-9565/ab8ebc is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1016/j.cpc.2012.02.021 is OK
- 10.1016/j.cpc.2012.11.019 is OK
- 10.22331/q-2020-10-11-341 is OK

MISSING DOIs

- 10.1063/5.0125683 may be a valid DOI for title: Quantum embedding of multi-orbital fragments using the block-Householder transformation

INVALID DOIs

- None
jarvist commented 1 year ago

@editorialbot recommend-accept

Let's give it another go :^), assuming changes are all pushed to GitHub.

editorialbot commented 1 year ago
Attempting dry run of processing paper acceptance...
editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1021/acs.jctc.7b00174 is OK
- 10.1063/5.0006074 is OK
- 10.21105/joss.00819 is OK
- 10.21468/scipostphys.2.1.003 is OK
- 10.21468/SciPostPhys.7.2.020 is OK
- 10.1088/2058-9565/ab8ebc is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1016/j.cpc.2012.02.021 is OK
- 10.1016/j.cpc.2012.11.019 is OK
- 10.22331/q-2020-10-11-341 is OK

MISSING DOIs

- 10.1063/5.0125683 may be a valid DOI for title: Quantum embedding of multi-orbital fragments using the block-Householder transformation

INVALID DOIs

- None
editorialbot commented 1 year ago

:wave: @openjournals/pe-eics, this paper is ready to be accepted and published.

Check final proof :point_right::page_facing_up: Download article

If the paper PDF and the deposit XML files look good in https://github.com/openjournals/joss-papers/pull/3810, then you can now move forward with accepting the submission by compiling again with the command @editorialbot accept

jarvist commented 1 year ago

🎉 Congratulations @SYalouz ! Many thanks reviewers. An Editor in Chief should be along for the final checks and balances.

SYalouz commented 1 year ago

Dear @jarvist , Dear @wcwitt and @erikkjellgren,

I would like to thank you all sincerely for your very constructive comments and suggestions that have improved the quality of the code and the paper.

A big thank you to all of you!

kyleniemeyer commented 1 year ago

Hi @SYalouz, can you please modify the metadata of the Zenodo archive such that the second author's name matches the paper? (I think currently it is using the GitHub handle.)

Also, I made a few typographic fixes to the paper and bibliography, can you merge the PR? https://github.com/SYalouz/QuantNBody/pull/6