Closed editorialbot closed 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
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1016/j.cpc.2018.03.016 is OK
- 10.1021/acs.jctc.8b00770.s001 is OK
- 10.1103/physrevmaterials.6.013804 is OK
- 10.21203/rs.3.rs-244137/v1 is OK
- 10.1016/j.cpc.2016.05.010 is OK
- 10.26434/chemrxiv.12218294 is OK
- 10.1145/3458817.3487400 is OK
- 10.1038/s41524-021-00617-2 is OK
- 10.1088/1741-4326/abe7bd is OK
- 10.1021/acs.jpca.0c02450.s001 is OK
- 10.1021/acs.jpca.9b08723.s001 is OK
- 10.1016/j.cpc.2021.108171 is OK
- 10.2172/1763572 is OK
- 10.1103/physrevlett.104.136403 is OK
- 10.1063/1.4712397 is OK
- 10.1007/s10853-021-06865-3 is OK
- 10.1063/1.5017641 is OK
- 10.1016/j.jcp.2014.12.018 is OK
- 10.1103/physrevb.99.014104 is OK
- 10.1016/j.commatsci.2018.07.043 is OK
MISSING DOIs
- None
INVALID DOIs
- None
Software report:
github.com/AlDanial/cloc v 1.88 T=20.15 s (1507.7 files/s, 79812.7 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
JSON 30182 1802 0 1577055
Python 141 3617 3614 13497
Markdown 35 402 0 1649
reStructuredText 11 420 101 952
Jupyter Notebook 1 0 3810 334
Cython 2 87 56 279
TeX 1 23 0 240
YAML 2 14 37 114
Bourne Shell 2 0 3 35
DOS Batch 1 8 1 26
make 1 4 7 9
C/C++ Header 1 3 15 3
-------------------------------------------------------------------------------
SUM: 30380 6380 7644 1594193
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repository
Wordcount for paper.md
is 2846
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Howdy @tpurcell90 and @bahung!
Thanks for agreeing to review this submission.
The process for conducting a review is outlined above. Please run the command shown above to have @editorialbot generate your checklist, which will give a step-by-step process for conducting your review. Please check the boxes during your review to keep track, as well as make comments in this thread or open issues in the repository itself to point out issues you encounter. Keep in mind that our aim is to improve the submission to the point where it is of high enough quality to be accepted, rather than to provide a yes/no decision, and so having a conversation with the authors is encouraged rather than providing a single review post at the end of the process.
Here are the review guidelines: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html And here is a checklist, similar to above: https://joss.readthedocs.io/en/latest/review_checklist.html
Please let me know if you encounter any issues or need any help during the review process, and thanks for contributing your time to JOSS and the open source community!
Hi @tpurcell90 and @bahung, hope all is well.
Feel free to try the Colab tutorial for installing FitSNAP and performing fits: https://colab.research.google.com/github/FitSNAP/FitSNAP/blob/master/tutorial.ipynb
This tutorial applies what is found in the docs: https://fitsnap.github.io/
Thanks, sorry for not getting to this sooner, I have been swamped the last few weeks, I hope to get to this by the end of the weekend.
Comments on the installation:
module purge
module load gcc
module load cmake
module load openmpi
module load intel # Sometimes this helps mpi4py work
I am confused why the intel module would be needed for mpi4py. mpi4py should use whatever MPI library/executable was active when pip install mpi4py
was run (conda is different and uses a conda-internal MPI library which can complicate things). My concern is that by loading both modules there could be an MPI mismatch between LAMMPS and FitSNAP that can lead to a failure in theory (I have not tested this explicitly)
-DPKG_ML-PACE=yes \
is in the non-PACE install instructions, but you have a separate set of instructions for PACE. Is this intentional?CMake Warning at CMakeLists.txt:208 (add_library):
Cannot generate a safe runtime search path for target lammps because files
in some directories may conflict with libraries in implicit directories:
runtime library [libz.so.1] in /usr/lib/x86_64-linux-gnu may be hidden by files in:
/home/tpurcell/anaconda3/envs/fitsnap/lib
runtime library [libgomp.so.1] in /usr/lib/gcc/x86_64-linux-gnu/9 may be hidden by files in:
/home/tpurcell/anaconda3/envs/fitsnap/lib
Some of these libraries may not be found correctly.
CMake Warning at CMakeLists.txt:215 (add_executable):
Cannot generate a safe runtime search path for target lmp because files in
some directories may conflict with libraries in implicit directories:
runtime library [libgomp.so.1] in /usr/lib/gcc/x86_64-linux-gnu/9 may be hidden by files in:
/home/tpurcell/anaconda3/envs/fitsnap/lib
Some of these libraries may not be found correctly.
This is a common issue when dealing with conda, I'll see if this causes runtime issues. If it doesn't you may want to add a blurb warning about this issue for a user who may not be as experienced.
-Weffc++
and -Wunused-result
warnings. Depending on who controls the LAMMPS code you might just want to recommend turning off those warnings in the installation instructions. They should not affect the runtime performance of the code.make install-python
I would just add a warning that if you did not create a separate virtual env for the code you may need sudo privileges (I ran into this issue in the past)site-packages
directory? Setting the PYTHONPATH works, but it is easier just to install the code using pip or python setup.py install(I will edit once I do the run FitSNAP options to see if I encounter any other issues)
Input File Examples
exceptionfitsnap3lib.parallel_tools.GracefulError(*args, **kwargs)
has no docstring. This maybe intentional, but worth noting
Otherwise the library API is good
For style guides you might want to add a .flake8
files and add a pylint or falke8 test in the build step to confirm the style guide is matched
Running the tests with intel-MPI fails with the following error:
match_arg (../../../../../src/pm/i_hydra/libhydra/arg/hydra_arg.c:91): unrecognized argument oversubscribe
I believe that this is an artifact of using openMPI arguments for a non-openMPI library. Should I recompile everything using openMPI?
In the collab notebook: For multi-element NNs section: the runs fail with the following traceback
/content/FitSNAP/examples/WBe_PyTorch_NN
Traceback (most recent call last):
File "plot_comparison.py", line 15, in <module>
nnt = NNTools("peratom.dat", "perconfig.dat")
File "/content/FitSNAP/fitsnap3lib/tools/nn_tools.py", line 32, in __init__
self.dfa = pd.read_csv(peratom_file,
File "/usr/local/lib/python3.8/dist-packages/pandas/util/_decorators.py", line 311, in wrapper
return func(*args, **kwargs)
File "/usr/local/lib/python3.8/dist-packages/pandas/io/parsers/readers.py", line 586, in read_csv
return _read(filepath_or_buffer, kwds)
File "/usr/local/lib/python3.8/dist-packages/pandas/io/parsers/readers.py", line 482, in _read
parser = TextFileReader(filepath_or_buffer, **kwds)
File "/usr/local/lib/python3.8/dist-packages/pandas/io/parsers/readers.py", line 811, in __init__
self._engine = self._make_engine(self.engine)
File "/usr/local/lib/python3.8/dist-packages/pandas/io/parsers/readers.py", line 1040, in _make_engine
return mapping[engine](self.f, **self.options) # type: ignore[call-arg]
File "/usr/local/lib/python3.8/dist-packages/pandas/io/parsers/c_parser_wrapper.py", line 51, in __init__
self._open_handles(src, kwds)
File "/usr/local/lib/python3.8/dist-packages/pandas/io/parsers/base_parser.py", line 222, in _open_handles
self.handles = get_handle(
File "/usr/local/lib/python3.8/dist-packages/pandas/io/common.py", line 702, in get_handle
handle = open(
FileNotFoundError: [Errno 2] No such file or directory: 'peratom.dat'
Run MD with WBe NN-SNAP fails with:
ERROR on proc 0: Cannot open SNAP parameter file ../WBe_pot.mliap.descriptor: No such file or directory (src/ML-IAP/mliap_descriptor_snap.cpp:379)
Last command: pair_style hybrid/overlay zbl 4.0 4.8 mliap model mliappy ../FitTorch_Pytorch.pt descriptor sna ../WBe_pot.mliap.descriptor
--------------------------------------------------------------------------
MPI_ABORT was invoked on rank 0 in communicator MPI_COMM_WORLD
with errorcode 1.
Pairwise netoworks fails with:
!python -m fitsnap3 Ta-example.in --overwrite
Average loss over batches is 0.006325966378100789
'decorated_perform_fit' took 1518653.77 ms on rank 0
'fit_gather' took 0.01 ms on rank 0
Traceback (most recent call last):
File "/content/FitSNAP/fitsnap3/__main__.py", line 48, in main
snap.perform_fit()
File "/content/FitSNAP/fitsnap3lib/fitsnap.py", line 152, in perform_fit
error_analysis()
File "/content/FitSNAP/fitsnap3lib/parallel_tools.py", line 277, in timed
result = method(*args, **kw)
File "/content/FitSNAP/fitsnap3lib/fitsnap.py", line 148, in error_analysis
self.solver.error_analysis()
File "/content/FitSNAP/fitsnap3lib/solvers/solver.py", line 338, in error_analysis
decorated_error_analysis()
File "/content/FitSNAP/fitsnap3lib/parallel_tools.py", line 308, in check_if_rank_zero
return method(*args, **kw)
File "/content/FitSNAP/fitsnap3lib/solvers/solver.py", line 184, in decorated_error_analysis
mae_e[c.group]["train"] += ae
KeyError: ''
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/usr/lib/python3.8/runpy.py", line 194, in _run_module_as_main
return _run_code(code, main_globals, None,
File "/usr/lib/python3.8/runpy.py", line 87, in _run_code
exec(code, run_globals)
File "/content/FitSNAP/fitsnap3/__main__.py", line 55, in <module>
main()
File "/content/FitSNAP/fitsnap3lib/parallel_tools.py", line 277, in timed
result = method(*args, **kw)
File "/content/FitSNAP/fitsnap3/__main__.py", line 51, in main
output.exception(e)
TypeError: exception() missing 1 required positional argument: 'err'
The above comments are the only issues I have found. @rohskopf Do you want me to make issues in the repo for all of these points or discuss them here? I think it will be easier for the discussions to be done as separate issues in the repo, but I'll leave that decision to you.
Thank you @tpurcell90 for the detailed review. I will start addressing your points.
Feel free to open issues on our repo as well.
Thank you for the review, @tpurcell90. There is no requirement to open issues in the repo, but I've found that opening issues can be useful for tracking the issue if the fix ends up being more complicated than expected, and allows others to comment on it if they encounter a similar problem in the future.
I added 6 issues and one optional issue for the code recommendations/actual errors I found all headed with JOSS Review prefix. The other comments aren't really issues and more questions so I did not add an issue onto the repo.
Thank you @tpurcell90. I am going through these this week. In the meantime I addressed these issues:
https://github.com/FitSNAP/FitSNAP/issues/184 regarding tutorial errors: I fixed the tutorial problem in https://github.com/FitSNAP/FitSNAP/pull/187, this was a small update to the tutorial for new NN outputs we recently introduced.
https://github.com/FitSNAP/FitSNAP/issues/180 regarding setup.py
:
I added a pyproject.toml
file in https://github.com/FitSNAP/FitSNAP/pull/187. Curious if you or @jmschrei have opinions on pyproject.toml
vs. setup.py
.
I have no opinion. I think setup.py
in has more general capabilities, but they both work in this case.
I'll check the tutorials later this week.
I don't have a strong opinion either way. I personally use setup.py
but that's because that was the trend when I learned how to do these things.
Hi @tpurcell90 and @jmschrei, quick update.
We addressed the issues @tpurcell90 raised, outlined below. The issues also show which PRs and commits were involved.
https://github.com/FitSNAP/FitSNAP/issues/180 : We added a pyproject.toml
file and uploaded our package to PyPI, so that users can install with pip.
https://github.com/FitSNAP/FitSNAP/issues/181 : We have an internal function that converts settings to the correct data type internally, regardless of whether 1/0 or True/False is used for settings.
https://github.com/FitSNAP/FitSNAP/issues/182 : I added flake8
linting in setup.cfg
for formatting specs that we care about, and added a note for contributors about using flake8
to verify proper format.
https://github.com/FitSNAP/FitSNAP/issues/183 : Our CI tests use OpenMPI so we never caught this. We use the tests to verify that the code gives consistent results and that our interatomic potentials are correctly coded, so Intel or OpenMPI was never a concern. I added a note in the test README about an OpenMPI requirement.
https://github.com/FitSNAP/FitSNAP/issues/184 : Colab tutorial is fixed, just required minor changes to use new NN file formats.
https://github.com/FitSNAP/FitSNAP/issues/185 : Since LAMMPS warnings are useful for developers at this time, and sometimes rely on external packages, it is best to keep these warnings. I therefore mentioned this in the installation docs so that users are not alarmed.
https://github.com/FitSNAP/FitSNAP/issues/186 : We have an example using our library combined with ASE, which also has scrapers. We prefer this instead of making ASE another dependency.
On the remaining checklist items for functionality:
I will note that the installation procedure is used in the Colab tutorial, but we also have pip
as an option now. As for functional claims, the purpose of our software is to learn a mapping of atomic positions to energies and forces, then deploy these models for simulation in LAMMPS. I think our tutorial illustrates this nicely. Our CI tests for NN potentials also confirm that forces are correctly coded by comparing analytical forces to finite difference forces.
Thank you @tpurcell90 for the diligent and descriptive issues that were raised. Please let me know if you would like to see anything different.
Hi @tpurcell90 , our tutorial installation broke with Colab's new Python version update. I'm fixing it now. Apologies if you tried it in the past day.
HI Drew, I'll take a look at the collab notebook once I am back from APS (next week). Assuming that everything works with OpenMPI then most of my concerns will likely be addressed and I can complete the check list.
The intel MPI issue and collab notebook problems were the two main reasons I could not check them off.
Hi @rohskopf and @tpurcell90
Sorry for joining late, I guess I should wait for @rohskopf fixing the tutorial before going on with my checklist.
Thanks Hung
Thanks @bahung and @tpurcell90 . Tutorial was fixed and merged in https://github.com/FitSNAP/FitSNAP/pull/192
My comments are as follows after the colab installation and running the author's provided colab notebook:
Thanks, Ba Hung
Thank you @bahung for these excellent points.
I will address them one by one as separate comments below, and make necessary changes in https://github.com/FitSNAP/FitSNAP/pull/194
- Installation: It has some 'declared with attribute warn_unused_result' warnings which are mainly from LAMMPS, I think we better have some notes in installation and/or documentation as @tpurcell90 has already mentioned
I added a small comment to the docs to tell users not to be warned: "Do not be alarmed by runtime library warnings after cmake, or -Weffc++ and -Wunused-result warnings during make." Also explained in https://github.com/FitSNAP/FitSNAP/issues/185
- Functionality: Most of the claims of FitSNAP are satisfied. It could be great if the uncertainty quantification (UQ), while described in the documentation, is also illustrated in the colab notebook. Thus I cannot verify the full functionality at the moment.
Thanks for pointing out the need for a UQ illustration. I added a UQ example to the colab notebook, in a section like this:
This outputs a covariance.npy
file that has covariance values for each fitting coefficient. I show in the tutorial that we can make parity plots with these uncertainties, e.g. here's a plot showing comparisons in energy between model and target including uncertainty bars:
I also added a more appropriate UQ section in the docs: https://fitsnap.github.io/Linear.html#uncertainty-quantification-uq
Here we explain the UQ method that we have implemented, an analytical Bayesian method that produces the output covariance values. We reserve a separate section for more experimental/exploratory methods here: https://fitsnap.github.io/Linear.html#in-development-uq-solvers
In the paper, we mention that our UQ solvers output covariances for each fitting coefficient, and that is what we added to the tutorial with the former method.
- Documentation: Section 4 Linear models with only one subsection seems a bit unnormal, please consider adding/separating how we fit linear models and so forth.
Thank you for pointing this out. I moved the UQ method to the linear section since our UQ only applies to linear models for now. We also show the exact fitting matrix we use, e.g. shown here:
Where all the symbols/definitions are given in https://fitsnap.github.io/Linear.html#
Then we also state:
FitSNAP solves this matrix problem using SVD and supports other solvers as well. For more details on settings used for descriptors and solvers, please see the docs on FitSNAP Input Scripts.
- Paper: I think the reference for [janssen2019pyiron] is missing, please include it in the paper.
Thanks for catching. Fixed.
- Tutorial: It has a detailed tutorial in Google Colab which is fine, it could be great if the colab notebook is more well-organised to present all FitSNAP functionalities. Eg. consistency of numberings, functions, or real-world examples.
Thanks for bringing this up. Now that UQ is added to the tutorial this completes all of our current functionalities. The rest of the examples are the same, but with different training data.
The purpose of our software is to learn a mapping between atomic positions and energies/forces extracted from quantum mechanics. Then deploy these models for running molecular dynamics simulations. The tutorial illustrates this purpose for some example materials involved in nuclear energy applications (these were also cited in the paper).
There are many real-world examples of applying molecular dynamics simulations to a variety of problems, such as calculating/predicting material properties which is very useful in materials engineering. These are best left to the paper with citations pointing towards the various publications that have used FitSNAP and/or molecular dynamics simulations.
More comments on consistency below.
- Tutorial: It has a detailed tutorial in Google Colab which is fine, it could be great if the colab notebook is more well-organised to present all FitSNAP functionalities. Eg. consistency of numberings, functions, or real-world examples.
More comments regarding consistency of numbers/results and functions:
To further satsify this, we added a Tests section to the docs which explains our CI tests: https://fitsnap.github.io/Tests.html
As seen there, our CI tests verify:
To further illustrate the correctness of neural network derivatives w.r.t. atomic positions, I added a section to the tutorial here:
which shows a distribution of errors between analytical/autodiff forces and finite difference forces:
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Thank you @rohskopf for the promtly responses. The functionalities of FitSNAP are now confirmed with elaborated examples. I completed my checklist.
Thank you @bahung and thanks again for the excellent suggestions.
@tpurcell90 do you have remaining concerns about this submission?
No sorry I have been travelling all month and forgot to confirm that everything is good with me
No problem, thanks for the update.
@rohskopf do you have a DOI for the code+paper, e.g. on Zenodo, and a version number to target for this submission?
@editorialbot check references
@editorialbot generate pdf
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1016/j.cpc.2018.03.016 is OK
- 10.1021/acs.jctc.8b00770.s001 is OK
- 10.1103/physrevmaterials.6.013804 is OK
- 10.21203/rs.3.rs-244137/v1 is OK
- 10.1016/j.cpc.2016.05.010 is OK
- 10.26434/chemrxiv.12218294 is OK
- 10.1145/3458817.3487400 is OK
- 10.1038/s41524-021-00617-2 is OK
- 10.1088/1741-4326/abe7bd is OK
- 10.1021/acs.jpca.0c02450.s001 is OK
- 10.1021/acs.jpca.9b08723.s001 is OK
- 10.1016/j.cpc.2021.108171 is OK
- 10.2172/1763572 is OK
- 10.1103/physrevlett.104.136403 is OK
- 10.1063/1.4712397 is OK
- 10.1007/s10853-021-06865-3 is OK
- 10.1063/1.5017641 is OK
- 10.1016/j.jcp.2014.12.018 is OK
- 10.1103/physrevb.99.014104 is OK
- 10.1016/j.commatsci.2018.07.043 is OK
MISSING DOIs
- 10.1038/s41467-023-36329-y may be a valid DOI for title: Learning Local Equivariant Representations for Large-Scale Atomistic Dynamics
INVALID DOIs
- None
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@rohskopf can you fix the missing DOI?
@jmschrei we don't have a DOI for the code+paper, is that required?
Version number is 3.0.1
Fixing DOI now.
Submitting author: !--author-handle-->@rohskopf<!--end-author-handle-- (Andrew Rohskopf) Repository: https://github.com/FitSNAP/FitSNAP Branch with paper.md (empty if default branch): Version: 3.0.1 Editor: !--editor-->@jmschrei<!--end-editor-- Reviewers: @tpurcell90, @bahung Archive: 10.5281/zenodo.7764752
Status
Status badge code:
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
@tpurcell90 & @bahung, 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:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @jmschrei 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 @tpurcell90
📝 Checklist for @bahung