[REVIEW]: mamonca - interactive magnetic Monte Carlo code

editorialbot commented 8 months ago

Submitting author: !--author-handle-->@samwaseda@kellyrowland<!--end-editor-- Reviewers: @arjunsavel, @vipinagrawal25 Archive: 10.5281/zenodo.13309692

Status

Status badge code:

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

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

@arjunsavel & @vipinagrawal25, 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 @kellyrowland 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 @vipinagrawal25

📝 Checklist for @arjunsavel

editorialbot commented 8 months 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 8 months ago

Software report:

github.com/AlDanial/cloc v 1.88  T=0.04 s (436.5 files/s, 56532.3 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C++                              1             99              0            754
C/C++ Header                     1             21              1            203
Cython                           2             51            197            173
Python                           5             37              0            147
TeX                              1             11              0            112
Markdown                         3             26              0             73
YAML                             3              6              5             61
Jupyter Notebook                 1              0            293             56
make                             1              1              0              4
-------------------------------------------------------------------------------
SUM:                            18            252            496           1583
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository

editorialbot commented 8 months ago

Wordcount for paper.md is 502

editorialbot commented 8 months ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1088/0953-8984/26/10/103202 is OK
- 10.1016/j.cpc.2021.107938 is OK
- 10.1051/jphystap:019170070010300 is OK
- 10.1073/pnas.97.18.9840 is OK
- 10.1016/j.commatsci.2018.07.043 is OK

MISSING DOIs

- None

INVALID DOIs

- None

editorialbot commented 8 months ago

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

vipinagrawal25 commented 8 months ago

Review checklist for @vipinagrawal25

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the https://github.com/samwaseda/mamonca?
[x] License: Does the repository contain a plain-text LICENSE or COPYING file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@samwaseda) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
[x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
[x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
[x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

arjunsavel commented 8 months ago

Review checklist for @arjunsavel

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the https://github.com/samwaseda/mamonca?
[x] License: Does the repository contain a plain-text LICENSE or COPYING file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@samwaseda) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
[x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
[x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
[x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

arjunsavel commented 8 months ago

Thanks to @samwaseda for submitting an interesting package! mamonca is a modular code for simulating magnetic interactions that fits in neatly with existing codes from the literature.

My comments are below — once they are addressed, I recommend this for publication.

Documentation notes

The documentation could benefit from a short description of how to run the tests (I successfully ran them).
The documentation could also benefit from a more detailed statement of need. I.e., why compute Metropolis Monte Carlo simulations?
I couldn't verify the conda installation (my os is arm64). Perhaps mention that only linux installation is currently supported for the conda install?
The cython dependency is not currently listed — setup.py fails without a cython install.
The C code of mamonca is largely uncommented. I understand that the docstrings are in the Python interface, but commenting through the C logic would make it more readable.
The package does not currently contain contributing guidelines. For an example, see the Numba contributing guidelines.
Pyiron is technically a dependency for running the provided examples — on that basis, I think that it should be added as a requirement.
Once we run a simulation, what are some of the properties we can get? This information is in the docstring(s), and it should also be in the Quickstart.
After executing the set_repeat method, I get the following error: DeprecationWarning: pyiron_atomistics.atomistics.structure.atoms.set_repeat is deprecated: Use Atoms.repeat. when running structure.set_repeat. Could this error be suppressed, circumvented, or explained?
In the "First Steps" in the README, each line should be explained in more depth. E.g., what does .set_repeat(10) do? Why 10? What do I do with the output? How do I know it’s run successfully? What things can go wrong?
When getting the mean energy, are these units also in eV? Same question about the energy variance.
The README could point toward the "First Steps" notebook for users interested in further examples.

Notebook Notes

The Curie temperature is noted as being “higher than it is supposed to be”. What is it supposed to be?
I got slightly different plots when I ran the notebook cells — could the authors emphasize somewhere in the documentation (ideally the README) that the Monte Carlo nature of the calculation will induce some random error between calculations? Is there a recommendation to run the code a number of times? By how much should I increase the number of iterations?
What parameters(s) did we choose randomly?
Just checking that the magnetization is indeed unitless.
As in the README, more commenting throughout would be helpful. For instance, what's happening in the loop over ti_lambda? What is fcc?
Ideally, under each heading, there’d be a description of what the goal of that subsection is.
What is the lambda being evaluated?

Paper notes

Line 7: Ranging from iron to what? The authors should include another example (e.g., "ranging from iron to copper") or leave iron as the only example (e.g., "for instance, iron")
Line 11: "...compute the potential energy" the authors could provide an example of what systems the potential energy is computed for — even if it's just a generic lattice.
Usually, the package itself and its features are described briefly in the Summary.
A brief description of the differences between the Monte Carlo method and the spin dynamics method would be helpful (especially if there are any obvious pros/cons to each).
Line 21: "on-the-fly" should not be hyphenated in this usage (because it doesn't precede the thing it's modifying)
The authors could describe the workflow with pyiron a bit more — e.g., what input parameters for the Hamiltonian are provided.
Are there specific benefits to running both metadynamics and thermodynamic integration in one code?
It might be helpful to include a plot showing typical output from mamonca.

kellyrowland commented 8 months ago

@vipinagrawal25 thanks for getting started here - please let me know if you have any questions or need to set the review down.

kellyrowland commented 7 months ago

@vipinagrawal25 thanks for getting started here - please let me know if you have any questions or need to set the review down.

kellyrowland commented 7 months ago

@samwaseda I see there were some updates to the code repo a few weeks ago - should @arjunsavel have another look at things, or are there further edits needed?

samwaseda commented 7 months ago

Thank you very much for the review! The points were so pertinent and striking that I had to take some time making the changes! I will leave a comment here as soon as I am done with it.

samwaseda commented 7 months ago

@arjunsavel Thank you very much again for your review! I could see how much effort you must have put to see all these points, and I am very grateful for your sincerity. It took me now some time to make corrections to the package for me, but I think now I looked at all the points and it should be ready. There's only one point that I decided to deviate from: I did not include a figure in the paper, because if it is a figure about the Magnetic Monte Carlo method, those who are working in the area know precisely what kind of figure this software package should be able to deliver. For other features, such as thermodynamic integration or Metadynamics, I did not know how to present the figures without going too much into details. I hope the fact they are presented in the example notebook is acceptable.

kellyrowland commented 7 months ago

hi @arjunsavel @vipinagrawal25 please give the code another look at your earliest convenience; there have been some revisions. if you have any questions or concerns or need to put the review down, please do let me know.

arjunsavel commented 7 months ago

@editorialbot generate pdf

editorialbot commented 7 months ago

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

arjunsavel commented 7 months ago

Thanks to @samwaseda for addressing my comments! The changes you made have made the package even cleaner.

I have a few (hopefully small) final requests:

in the paper: "while spin dynamics would deliver also the kinetics —> while spin dynamics also delivers the kinetics"
What are the units (if any) for "lambda" in the first steps notebook?
What do you think of adding pyiron to the setup.py requirements?
Could follow one of the suggestions here so that Cython isn't imported in setup.py before it's installed

Also, the LaTeX in the readme is a nice touch!

samwaseda commented 7 months ago

Thanks again for the comments! Now I made changes accordingly!

What are the units (if any) for "lambda" in the first steps notebook?

I added "unitless" in the notebook

What do you think of adding pyiron to the setup.py requirements?

That's in principle a great idea (and sorry for not commenting on this in my last entry). One main problem is that it takes a very long time to install pyiron, so those who don't need pyiron would definitely not want to install pyiron for no reason. I added the comments in README saying how to install pyiron, so I'm hoping it will be enough. What do you think?

Could follow one of the suggestions here so that Cython isn't imported in setup.py before it's installed

Thanks for your support! I didn't know much about this part, but now I changed it according to what's written there. I hope now it works!

arjunsavel commented 7 months ago

Thanks for your quick response!

That's in principle a great idea (and sorry for not commenting on this in my last entry). One main problem is that it takes a very long time to install pyiron, so those who don't need pyiron would definitely not want to install pyiron for no reason. I added the comments in README saying how to install pyiron, so I'm hoping it will be enough. What do you think?

This makes sense. Thank you for the explanation!

Thanks for your support! I didn't know much about this part, but now I changed it according to what's written there. I hope now it works!

Thanks for working on this! I think the last thing to do, in addition to the setuptools version pin, is something like the below:

from setuptools.command.build_ext import build_ext
from setuptools import setup, Extension

ext = Extension(
    'mamonca',
    sources=["mamonca/mc.pyx"],
    language="c++",
    extra_compile_args=['-std=c++11'],
)

with open('README.md') as readme_file:
    readme = readme_file.read()

setup(
    name='mamonca',
    version='0.0.8',
    description='mamonca - interactive Magnetic Monte Carlo code',
    long_description=readme,
    long_description_content_type='text/markdown',
    url='https://github.com/samwaseda/mamonca',
    author='Sam Waseda',
    author_email='waseda@mpie.de',
    license='BSD',
    cmdclass={"build_ext": build_ext},
    ext_modules=[ext],
    options={'build': {'build_lib': 'mamonca'}},
    setup_requires=[
        # Setuptools 18.0 properly handles Cython extensions.
        'setuptools>=18.0',
        'cython',
        'numpy',
    ],
)

This way, Cython never needs to be imported at the top of the file, but setuptools is told that it requires Cyython to build.

samwaseda commented 7 months ago

Thanks again! Now I made the changes and committed them!

vipinagrawal25 commented 7 months ago

Dear @samwaseda,

Firstly, please accept my apologies for the delay in completing the review of your paper. Thanks for the submission. I also would like to thank the editor @kellyrowland for choosing me as a reviewer and @arjunsavel for the thorough review.

The mamonca package is built for computing free-energy for materials using Heisenberg Landau model. Additionally the package offers magnetic thermodynamic integration and metadynamics. I think the community will be benefitted from such software hence I recommend this for publication provided the comments outlined below are addressed.

General:

I have observed that most commits are made by @samwaseda, followed by Simon Bekemeier with 4 commits, and Jan Janssen with one commit. However the paper does not include these names in the co-author list but two other names which have not contributed to the repository. In case, if these co-authors have made non-code contributions to the work, I recommend including an author contribution section. Also note that, the Joss guidelines emphasise that,

Purely financial (such as being named on an award) and organizational (such as general supervision of a research group) contributions are not considered sufficient for co-authorship of JOSS submissions, but active project direction and other forms of non-code contributions are.

Documentation:

It would be beneficial to include information about dependencies in the README file. For instance, user should install Cython before using the package.
While the mc.pyx file contain good comments, I suggest adding comments to cMC.cpp as well. Additionally, the use of Doxygen could benefit the user.

Examples:

I was unable to locate any examples on spin-dynamics. Can you please consider adding one example on that as well?
While the tests provided by authors are running as expected, additional information is needed to verify the correctness of results. The authors could include ways to benchmark the code. For instance:
- Compute statistical quantities to validate the results from MC simulation.
- Reproduce well-documented results.
- Compare results with those obtained from other existing codes.
- Similar benchmarks should be considered for magnetic thermodynamics integration and Metadynamics.

Paper:

It would be useful to include a statement about the target community. Are the intended users experimental material scientists running small simulations, or individuals involved in modeling?
Regarding the reference for understanding magnetic thermodynamic integration by Frenkel & Smit, please cite the specific chapter that explains the relevant part.
The paper could be enhanced by including a section on explaining the Heisenberg-Landau model, detailing how MC methods and spin dynamics are employed for such systems. Adding a figure would be helpful for better understanding. @samwaseda has already given explanation about not including figures related to thermodynamic integration or Metadynamics but they can consider including the plots for the former part.
Can you also explain how to use different Hamiltonian with this code?

kellyrowland commented 6 months ago

hi @samwaseda 👋 just checking in here on how things are going and if you're able to address the above feedback.

samwaseda commented 6 months ago

Hi! Sorry it's taking some time, but I will hopefully finish the revision this week! Thanks for waiting!

kellyrowland commented 6 months ago

no problem, thanks for the update!

kellyrowland commented 5 months ago

hi @samwaseda just checking in here 👋 no worries if updates are still in progress.

samwaseda commented 5 months ago

Yes! Sorry it's still taking some time...

kellyrowland commented 5 months ago

hi @samwaseda 👋 checking in again.

kellyrowland commented 4 months ago

hi @samwaseda 👋 checking in - no problem if edits are taking some time, just a ping here.

samwaseda commented 4 months ago

Hi! Sorry I'm working on it. My main problem is how to reproduce well documented results mentioned by @vipinagrawal25. The results I can find in the literature most don't give the numerical data needed to reproduce them. I keep looking for more reliable literature.

kellyrowland commented 4 months ago

no problem at all - @vipinagrawal25 would you be able to provide any pointers?

vipinagrawal25 commented 4 months ago

@editorialbot generate pdf

editorialbot commented 4 months ago

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

vipinagrawal25 commented 4 months ago

@samwaseda I realise that it can be a lot of work to do that. I had mentioned four points to verify the correctness of results.

1) Compute statistical quantities to validate the results from MC simulation. 2) Reproduce well-documented results. 3) Compare results with those obtained from other existing codes. 4) Similar benchmarks should be considered for magnetic thermodynamics integration and Metadynamics.

Can you please tell me which of these steps have been completed and which are pending?

samwaseda commented 4 months ago

Basically 2 is the main problem, because I haven't really been able to find well-documented results. There are indeed results, but in most of the cases they don't provide their parametrisation.

vipinagrawal25 commented 4 months ago

Okay. Please submit your report without including that particular point. I shall review it, and assess if it meets our requirements. As I'll be on vacation for the next 10 days, there's no rush to complete it.

kellyrowland commented 3 months ago

thanks for the work here all - just checking in on things @samwaseda 👋