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]: AQCNES: A Quasi-Continuum Non-Equilibrium Solver #7068

Open editorialbot opened 1 month ago

editorialbot commented 1 month ago

Submitting author: !--author-handle-->@shasax<!--end-author-handle-- (Shashank Saxena) Repository: https://gitlab.com/qc-devs/aqcnes Branch with paper.md (empty if default branch): joss_submission Version: v1.0.0 Editor: !--editor-->@philipcardiff<!--end-editor-- Reviewers: @streeve, @T-Hageman Archive: Pending

Status

status

Status badge code:

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

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

@streeve & @T-Hageman, 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 @philipcardiff 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 @streeve

📝 Checklist for @T-Hageman

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

Software report:

github.com/AlDanial/cloc v 1.90  T=1.55 s (276.8 files/s, 283985.2 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
F#                               9              0              0         348057
SVG                              2              2              2          41174
C/C++ Header                   142           3849           5301          20514
C++                            182           1391           3520           8780
JSON                            21              1              0           2665
CMake                           42            159            643           1156
Markdown                         8            233              0            910
Python                           8            127            183            612
YAML                             3             19             48            208
TeX                              1             36              4            177
Bourne Shell                     7             23            110            141
Dockerfile                       1              6             18             49
TOML                             2              7             30             44
CSV                              1              0              0              2
-------------------------------------------------------------------------------
SUM:                           429           5853           9859         424489
-------------------------------------------------------------------------------

Commit count by author:

   699  Gerhard Bräunlich
   252  Prateek
   252  pgupta@von-neumann
   133  ssaxena
   110  Stefan Zimmermann
   109  Manuel Weberndorfer
    50  oliver
    29  Shashank Saxena
    21  Prateek Gupta
    18  webmanue
    11  migspi
    10  migsp
     9  mspinola
     7  miguelspi
     6  Miguel
     3  Miguel Angel Spinola Fandila
     3  miguel.spinola@mavt.ethz.ch
     2  Prateek-Leibniz
     2  brgerhar
     2  miguel spinola
     1  annywang
     1  bkoelle
     1  blackspur
     1  pgupta
editorialbot commented 1 month ago

Paper file info:

📄 Wordcount for paper.md is 1287

✅ The paper includes a Statement of need section

editorialbot commented 1 month ago

License info:

🟡 License found: Other (Check here for OSI approval)

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

OK DOIs

- None

MISSING DOIs

- 10.1016/j.mechmat.2023.104681 may be a valid DOI for title: GNN-assisted phase space integration with applicat...
- 10.1016/j.commatsci.2022.111511 may be a valid DOI for title: A fast atomistic approach to finite-temperature su...
- 10.1016/j.jmps.2021.104495 may be a valid DOI for title: Nonequilibrium thermomechanics of Gaussian phase p...
- 10.1115/1.4023013 may be a valid DOI for title: Finite-temperature quasi-continuum
- No DOI given, and none found for title: The quasicontinuum method. Modeling microstructure...
- 10.1088/0965-0393/17/5/053001 may be a valid DOI for title: A unified framework and performance benchmark of f...
- 10.1016/j.cpc.2020.107315 may be a valid DOI for title: MXE: A package for simulating long-term diffusive ...
- 10.1103/physrevlett.97.170201 may be a valid DOI for title: Structural relaxation made simple
- No DOI given, and none found for title: The quasicontinuum method: Overview, applications ...
- 10.1016/j.commatsci.2015.10.050 may be a valid DOI for title: Nonequilibrium free-energy calculation of solids u...
- 10.1103/physrevb.71.094104 may be a valid DOI for title: Atomistic calculations of elastic properties of me...
- 10.1016/j.actamat.2022.118006 may be a valid DOI for title: Examination of computed aluminum grain boundary st...
- No DOI given, and none found for title: Finite-temperature grain boundary properties from ...
- No DOI given, and none found for title: Martensite-austenite transition and phonon dispers...
- 10.1038/123204a0 may be a valid DOI for title: The nature of martensite
- 10.1063/1.448024 may be a valid DOI for title: New Monte Carlo method to compute the free energy ...
- 10.1557/jmr.2010.0061 may be a valid DOI for title: Thermomechanical properties dependence on chain le...
- 10.1016/j.mechmat.2013.07.021 may be a valid DOI for title: Stochastic predictions of bulk properties of amorp...
- No DOI given, and none found for title: A fully-nonlocal energy-based formulation and high...

INVALID DOIs

- doi:10.4231/D3X05XD4W is INVALID (failed connection)
editorialbot commented 1 month ago

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

streeve commented 1 month ago

Review checklist for @streeve

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

T-Hageman commented 1 month ago

Review checklist for @T-Hageman

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

streeve commented 1 month ago

@shasax I am unsure the best way to interact with the repo (e.g. open issues) since I do not have an ETH gitlab account - please advise

danielskatz commented 1 month ago

@philipcardiff - Please see https://joss.readthedocs.io/en/latest/review_criteria.html#where-can-the-repository-be-hosted. This needs to be addressed by the authors.

philipcardiff commented 1 month ago

@shasax I am unsure the best way to interact with the repo (e.g. open issues) since I do not have an ETH gitlab account - please advise

@philipcardiff - Please see https://joss.readthedocs.io/en/latest/review_criteria.html#where-can-the-repository-be-hosted. This needs to be addressed by the authors.

Hi @shasax, I missed this when I initially reviewed the repository. Can you confirm if outside users can freely open issues in the current repository? If not, you would need to move the repository to a fully public location as per https://joss.readthedocs.io/en/latest/review_criteria.html#where-can-the-repository-be-hosted.

Thanks

shasax commented 1 month ago

Hi @philipcardiff, Thank you for your comment! I think the best way to do this would be to mirror our repository on a public platform. Here is the link to the mirror: https://gitlab.com/qc-devs/aqcnes

I have some questions about the review process: 1) Can we somehow update the link to our repository in this review issue to be the one above? 2) Also, can I invite another co-author on this issue who could help me in the review responses?

Regards, Shashank Saxena

philipcardiff commented 1 month ago

@editorialbot set https://gitlab.com/qc-devs/aqcnes as repository

editorialbot commented 1 month ago

Done! repository is now https://gitlab.com/qc-devs/aqcnes

philipcardiff commented 1 month ago

Hi @philipcardiff, Thank you for your comment! I think the best way to do this would be to mirror our repository on a public platform. Here is the link to the mirror: https://gitlab.com/qc-devs/aqcnes

I have some questions about the review process:

  1. Can we somehow update the link to our repository in this review issue to be the one above?
  2. Also, can I invite another co-author on this issue who could help me in the review responses?

Regards, Shashank Saxena

Hi, Shashank.

Mirroring the repository works.

  1. Done.
  2. Yes, of course, all co-authors (and others) are welcome to comment: this is a public review process.
streeve commented 1 month ago

Thanks all, I've opened a few initial issues on the new repo - I'll continue once I can work through my local build issues

g-braeunlich commented 1 month ago

Thanks! Will discuss your issues in the team.

shasax commented 3 weeks ago

Thanks for your issues. Here are some updates:

@streeve: We are working on your issues in separate merge requests in the repo.

@T-Hageman: We are removing the ETH login requirement in one of the merge requests in the repo. If you are using ubuntu and building the dependencies locally, it would be easier to use the bash commands in the file container/Dockerfile. Which system are you using exactly?

streeve commented 3 weeks ago

Thanks, @shasax. As the build/test/run issues are worked out here are my suggestions on the manuscript:

shasax commented 1 week ago

Thanks to the reviewers for their comments! We have tried to incorporate the suggestions in the revised manuscript, which has now been uploaded to the joss_submission branch of the current repo. The following points list the changes made according to the comments, for both reviewers. The comments are in bold, while the responses are in normal text.

@streeve : 1) The current Statement of Need contains both the current capabilities/approach and the reasons the software is needed. Separating into different sections will help clarify. Thank you for the suggestion. We have now separated the statement of need from the functionality, which is a new section added to the revised manuscript.

2) References which include Miller seems to be ill-formed Thank you for pointing this out. This has been fixed in the revised manuscript.

3) You mention “amorphous materials at atomic resolution” as compared to leveraging the crystal symmetries for spatial coarse graining. What are the limits (e.g. polycrystalline systems, molecules, etc.) that are or are not possible? Spatial coarse graining is possible only in regions of crystalline long-range order (including molecular systems). This would include regions inside a grain and away from defects such as dislocations or grain boundaries. However, any amorphous system would need to be simulated with atomic resolution. We have added a statement in the statement of need section to clarify this. There has been recent work on extending the QC method to amorphous solids, but this adaption goes far beyond the scope of the present software.

4) More detail of the computational capabilities, dependencies, and infrastructure would be useful (MPI, user documentation, unit test suite, CI, etc.) Thank you for the suggestion. Would you suggest these details be added to the manuscript or the docs / README.md ?

5) It would be helpful to clarify the values reported from Saxena et al. 2022: “approximately fifty-fold reduction in time to solution while using five-fold fewer computational resources together with a 6-fold reduction in number of atoms” Thank you for the suggestion. We have reproduced a small part of the table of computational times from Saxena et al. (2022) in the Example Applications section.

@T-Hageman : 1) Basic functions are documented, but it is unclear what models are used, as the relevant equations are not provided (instead, the authors refer to some of their previous publications) Thank you for the comment. We have added a new section named ‘Functionality’ to the revised manuscript, which summarizes the theory and models used.

2) No clear guidelines are directly provided, instead the documentation suggests reaching out to the developers A detailed set of community guidelines has now been added in a separate file called CONTRIBUTING.md in the repository.

3) The description of the code functionality is included in the summary, but could be clearer. Currently the summary is mainly repeating the statement of need (detailing what others have done), not what this code is doing itself. Thank you for the suggestion. We have tried to slightly modify and expand the code capabilities in the summary. However, we would like to point out that this is explained in detail in the statement of need.

4) While the statement of need section is well-written, the example application section may require expanding. It currently only briefly summarised which other works in literature have used this code to produce publications. Consider adding a few representative results/Figures to more clearly convey the applications the code is designed to handle (also to non-specialists), and what outputs/useful results it can produce. Thank you for the suggestion. We have now added a representative figure for each of the example applications mentioned in the manuscript.

5) Some statements in the text could use references, e.g. "Current atomic simulation techniques describe the entire ensemble as a collection of particles, each having a position and velocity in the three-dimensional space. This fully refined spatial representation restricts the possible dimensions of the ensemble to microscopic scales." (contradicts later statements about methods such as QC and GPP, could use a reference to a review article stating this, or weakening the statement to indicate that only most methods, not all methods, use this technique) and " Therefore, research in the past decades has focused on multiscale modeling techniques to advance atomistic simulations to larger length scales and longer time scales." (Please cite some of this past research (or a review article summarising this) to more clearly place the current work in context ) We apologize for the confusion here. The “current atomic simulation techniques” referred to in the former line point to molecular dynamics, which is a fully refined spatial representation. QC and GPP are the techniques employed in this software, AQCNES, to overcome the length and time scale limitations of molecular dynamics. We have added references in the statement of need, which summarize the multiscale modeling efforts to advance atomistic simulations to larger length scales and longer time scales.

We would also request reviewer @T-Hageman to let us know if the installation works now after we removed the ETH login requirement for the docker container.

streeve commented 1 week ago

@editorialbot generate pdf

editorialbot commented 1 week ago

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

T-Hageman commented 1 week ago

We would also request reviewer @T-Hageman to let us know if the installation works now after we removed the ETH login requirement for the docker container.

Thank you, yes, the updated docker install instructions work. Updated the reviewer checklist above to indicate that all major concerns have been addressed by the authors

streeve commented 1 week ago 
1. **The current Statement of Need contains both the current capabilities/approach and the reasons the software is needed. Separating into different sections will help clarify.**
   Thank you for the suggestion. We have now separated the statement of need from the functionality, which is a new section added to the revised manuscript.

This is much improved, but the section still contains many things that are not related to the "need". The initial reiteration of what aqcnes does seems out of place, but that may just be my preference. I would also put the more detailed description of aqcnes capabilities in the following section (how it fills the gap of the current needs). The mention of the API documentation should certainly be in another section. 

2. **References which include Miller seems to be ill-formed**
   Thank you for pointing this out. This has been fixed in the revised manuscript.

Many of the references are missing the [ ] 

3. **You mention “amorphous materials at atomic resolution” as compared to leveraging the crystal symmetries for spatial coarse graining. What are the limits (e.g. polycrystalline systems, molecules, etc.) that are or are not possible?**
   Spatial coarse graining is possible only in regions of crystalline long-range order (including molecular systems). This would include regions inside a grain and away from defects such as dislocations or grain boundaries.  However, any amorphous system would need to be simulated with atomic resolution. We have added a statement in the statement of need section to clarify this. There has been recent work on extending the QC method to amorphous solids, but this adaption goes far beyond the scope of the present software.

Thanks for this clarification

4. **More detail of the computational capabilities, dependencies, and infrastructure would be useful (MPI, user documentation, unit test suite, CI, etc.)**
   Thank you for the suggestion. Would you suggest these details be added to the manuscript or the docs / README.md ?

I think a brief mention of these things is useful in both places. As mentioned in point 1) the API documentation and examples are details that could be combined with this information for another section 

5. **It would be helpful to clarify the values reported from Saxena et al. 2022: “approximately fifty-fold reduction in time to solution while using five-fold fewer computational resources together with a 6-fold reduction in number of atoms”**
   Thank you for the suggestion. We have reproduced a small part of the table of computational times from Saxena et al. (2022) in the Example Applications section.

Thanks

  1. It's not clear to me in the summary that other upscaling techniques are generally not able to do finite temperature prediction
shasax commented 2 days ago

Thank you for the comments @streeve ! We have updated the manuscript again, addressing the comments above. Here is a summary of the changes: 1) A statement has been added to the summary, to clarify which techniques can/cannot do finite temperature and what AQCNES is capable of. 2) The first line in the statement of need (which straightaway introduced AQCNES) is now shifted down below 2 paragraphs, after discussing the shortcomings of the current approaches. 3) The API documentation statement has been removed from the statement of need and is now combined with a list of dependencies / CI information at the end of the Examples section.

I have a question about the comment regarding the missing [] in references. Could you point to such references? I followed the JOSS documentation of citing an article with @... However, this renders the citation within parenthesis and not square brackets. But, this seems to be common in all JOSS articles.

shasax commented 2 days ago

@editorialbot generate pdf

editorialbot commented 2 days ago

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

streeve commented 2 days ago 
1. A statement has been added to the summary, to clarify which techniques can/cannot do finite temperature and what AQCNES is capable of.

Thanks

2. The first line in the statement of need (which straightaway introduced AQCNES) is now shifted down below 2 paragraphs, after discussing the shortcomings of the current approaches.

Looks great

3. The API documentation statement has been removed from the statement of need and is now combined with a list of dependencies / CI information at the end of the Examples section.

This is very nice - was there intended to be a separate section heading? And minor point, but I always assume that the dependency versions are required unless noted otherwise (tested version is I believe how it's described in the docs)

I have a question about the comment regarding the missing [] in references. Could you point to such references? I followed the JOSS documentation of citing an article with @... However, this renders the citation within parenthesis and not square brackets. But, this seems to be common in all JOSS articles.

For the references I believe Tembhekar is one example where the brackets are missing in the markdown (causing only the year to be in the link)

shasax commented 1 day ago 
This is very nice - was there intended to be a separate section heading? And minor point, but I always assume that the dependency versions are required unless noted otherwise (tested version is I believe how it's described in the docs)

No, there was no intention of making a separate section heading for this. Yes, it is true that the dependency versions are required. CMake checks this for every package. I am confused about your point regarding the tested version.

For the references I believe Tembhekar is one example where the brackets are missing in the markdown (causing only the year to be in the link)

This difference comes from citing within the text, using @.... vs. citing as [@...]. It is similar to the difference between using \citet{} and \cite{} in LaTeX.

streeve commented 1 day ago 
This is very nice - was there intended to be a separate section heading? And minor point, but I always assume that the dependency versions are required unless noted otherwise (tested version is I believe how it's described in the docs)

No, there was no intention of making a separate section heading for this. Yes, it is true that the dependency versions are required. CMake checks this for every package. I am confused about your point regarding the tested version.

It don't think it fits in "Examples", but if you wanted to avoid a new section it could go within "Functionality"

For dependencies, I generally expect "dependency >= minimum_vesion", but recalled in the documentatio something about these versions being the ones directly tested in the current CI. I should have requested clarification in text that these are (I presume) the current tested, but not necessarily the minimum supported versions

For the references I believe Tembhekar is one example where the brackets are missing in the markdown (causing only the year to be in the link)

This difference comes from citing within the text, using @.... vs. citing as [@...]. It is similar to the difference between using \citet{} and \cite{} in LaTeX.

My mistake - I'm still not used to this ref style

shasax commented 15 hours ago

We have now created a new section titled Dependency and API documentation and moved these details there. A statement about the dependency versions being not strictly the minimum versions required but the ones that work well has also been added to the same section.

We would also like to know if there is something left to be checked/tested regarding the functionality and performance of the software?

shasax commented 15 hours ago

@editorialbot generate pdf

editorialbot commented 15 hours ago

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

streeve commented 3 hours ago

@shasax you and the authors have addressed everything from my side. I would recommend this for publication as is @philipcardiff