openjournals / joss-reviews

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

[REVIEW]: ComFiT: a Python library for computational field theory with topological defects #6599

Closed editorialbot closed 4 months ago

editorialbot commented 6 months ago

Submitting author: !--author-handle-->@vidarsko<!--end-author-handle-- (Vidar Skogvoll) Repository: https://github.com/vidarsko/ComFiT Branch with paper.md (empty if default branch): Version: v1.5.0 Editor: !--editor-->@lucydot<!--end-editor-- Reviewers: @flokno, @sharanroongta, @sbacchio Archive: 10.5281/zenodo.11395524

Status

status

Status badge code:

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

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

@flokno & @sharanroongta & @sbacchio, 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 @sharanroongta

📝 Checklist for @sbacchio

📝 Checklist for @flokno

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

Software report:

github.com/AlDanial/cloc v 1.90  T=0.13 s (1523.6 files/s, 273924.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                         161           3034           2466           6570
Markdown                        13           1011              0           2800
SVG                              3              2              2            896
Jupyter Notebook                15              0          17614            681
TeX                              1             32              0            240
YAML                             4             31              6            195
CSS                              1              2              1             16
-------------------------------------------------------------------------------
SUM:                           198           4112          20089          11398
-------------------------------------------------------------------------------

Commit count by author:

   480  Vidar Skogvoll
   278  jonasron
    77  Jonas Rønning
     2  MilosJoks
     1  Harish P Jain
     1  Milos Joksimovic
editorialbot commented 6 months ago

Paper file info:

📄 Wordcount for paper.md is 920

✅ The paper includes a Statement of need section

editorialbot commented 6 months ago

License info:

✅ License found: MIT License (Valid open source OSI approved license)

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

OK DOIs

- 10.1103/PhysRevLett.78.401 is OK
- 10.1103/PhysRevE.59.1574 is OK
- 10.1103/PhysRevE.85.011153 is OK
- 10.1006/jcph.2002.6995 is OK
- 10.1103/PhysRevE.95.052144 is OK
- 10.1103/PhysRevLett.121.255501 is OK
- 10.1103/PhysRevE.93.042137 is OK
- 10.1103/PhysRevE.93.032106 is OK
- 10.1103/PhysRevB.103.014107 is OK
- 10.1103/PhysRevB.103.224107 is OK
- 10.1016/j.jmps.2022.104932 is OK
- 10.1088/1361-651X/ac9493 is OK
- 10.1038/s41524-023-01077-6 is OK
- 10.1039/D3SM00316G is OK
- 10.1088/1367-2630/ab95de is OK
- 10.1103/PhysRevResearch.5.023108 is OK

MISSING DOIs

- No DOI given, and none found for title: A Unified Perspective on Two-Dimensional Quantum T...
- No DOI given, and none found for title: Symmetry, Topology, and Crystal Deformations: A Ph...
- No DOI given, and none found for title: Topological Defects and Flows in BECs and Active M...

INVALID DOIs

- None
editorialbot commented 6 months ago

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

sharanroongta commented 6 months ago

Review checklist for @sharanroongta

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

sbacchio commented 6 months ago

Review checklist for @sbacchio

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

vidarsko commented 6 months ago

Thanks for your review @sbacchio. I have added a second paragraph to the Statement of need to try and adress the state of the field. Let me know if you do not find it satisfactory and I will try to amend accordingly.

vidarsko commented 6 months ago

@editorialbot generate pdf

editorialbot commented 6 months ago

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

vidarsko commented 6 months ago

@lucydot I realize that I have not capitalized the p in "Python" for the submission on the JOSS website. Is it possible to amend that?

sharanroongta commented 6 months ago

@vidarsko Thanks for adding the state of the art. Is it also possible to add references for other open-source library for solving PDE?

vidarsko commented 6 months ago

@sharanroongta Yes, I have added the references to for mentionned packages.

vidarsko commented 6 months ago

@editorialbot generate pdf

editorialbot commented 6 months ago

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

sbacchio commented 6 months ago

@vidarsko, Thank you for promptly addressing the missing item. The added paragraphs are satisfactory to me. I warmly recommend the work for publication! It is a nice package with a very good documentation and examples.

I have only a marginal suggestion: you should add the example you provided in the manuscript to the tests and CI, such that you guarantee it will always be supported in the future :)

lucydot commented 6 months ago

I realize that I have not capitalized the p in "Python" for the submission on the JOSS website. Is it possible to amend that?

Do you mean in the issue thread title @vidarsko? I have just updated.

vidarsko commented 6 months ago

I realize that I have not capitalized the p in "Python" for the submission on the JOSS website. Is it possible to amend that?

Do you mean in the issue thread title @vidarsko? I have just updated.

Thanks for updating. I suppose the p in the issue thread is not capitalized because I forgot to do it in the submission on the JOSS website. This is how my pending submission looks like on the website.

image

lucydot commented 6 months ago

Thanks for your timely review @sbacchio ✨ - can you confirm that as part of the review you were able to install and run the package successfully? I ask as it is unusual for there to be no hiccups, so want to double check..

sharanroongta commented 5 months ago

@vidarsko Please have a look at https://github.com/vidarsko/ComFiT/issues/9

sbacchio commented 5 months ago

Thanks for your timely review @sbacchio ✨ - can you confirm that as part of the review you were able to install and run the package successfully? I ask as it is unusual for there to be no hiccups, so want to double check..

@lucydot, sure I've been able to install it on my machine and reproduce examples and tutorials. At least with my Python version, it worked out fine!

flokno commented 5 months ago

Review checklist for @flokno

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

flokno commented 5 months ago

@editorialbot generate pdf

editorialbot commented 5 months ago

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

vidarsko commented 5 months ago

@vidarsko Please have a look at vidarsko/ComFiT#9

We have addressed and closed the issue. Feel free to raise another issue if you find other problems and or are not satisfied with the resolution.

vidarsko commented 5 months ago

Note: I have added the custom domain https://comfitlib.com/ to the repository and changed the links in the paper and relevant sections of the code repository. Hope that is okay.

flokno commented 5 months ago

@vidarsko regarding the summary in the paper (Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?):

For my taste it is a bit too general for a non-expert in PDEs. Maybe you could name one or several widely-known PDEs and physical areas as examples right in the introduction (e.g. Navier-Stokes for fluid dynamcis?)

vidarsko commented 5 months ago

@vidarsko regarding the summary in the paper (Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?):

For my taste it is a bit too general for a non-expert in PDEs. Maybe you could name one or several widely-known PDEs and physical areas as examples right in the introduction (e.g. Navier-Stokes for fluid dynamcis?)

Thank you for your comment, @flokno. I have elaborated a little in the Statement of need to include some examples of famous PDEs.

vidarsko commented 5 months ago

@editorialbot generate pdf

editorialbot commented 5 months ago

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

sharanroongta commented 5 months ago

Hi @vidarsko, Here you have mentioned about two examples - one for scalar order parameter, and the other for a vector. I don't see any example for vector order parameter. Am I missing something?

vidarsko commented 5 months ago

Hi @vidarsko, Here you have mentioned about two examples - one for scalar order parameter, and the other for a vector. I don't see any example for vector order parameter. Am I missing something?

Thanks for pointing this out! I have extended the tutorial to include an example with a vector-valued order parameter.

sharanroongta commented 5 months ago

@vidarsko From my side, it is all fine now.

I just have a suggestion, the documentation is adequate but could be improved. I know it's an iterative process. The BaseSystem Class can be better explained in terms of how one could set up pde (like anisotropic heat conduction). I am happy with the clarification you have provided in the issue I had raised.

Thanks and all the best! :)

lucydot commented 5 months ago

Thanks @sharanroongta ✨

@flokno how is your review going?

flokno commented 5 months ago

@vidarsko regarding the summary in the paper (Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?): For my taste it is a bit too general for a non-expert in PDEs. Maybe you could name one or several widely-known PDEs and physical areas as examples right in the introduction (e.g. Navier-Stokes for fluid dynamcis?)

Thank you for your comment, @flokno. I have elaborated a little in the Statement of need to include some examples of famous PDEs.

Thank you @vidarsko . I find "Maxwell's equations which dictate how electromagnetic fields encode those well-crafted cat memes" a little bit too colloquial and confusing, but that's just a comment.

I have found some problems when running the tutorials locally and not in the cloud, please see https://github.com/vidarsko/ComFiT/issues/13

vidarsko commented 5 months ago

@vidarsko regarding the summary in the paper (Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?): For my taste it is a bit too general for a non-expert in PDEs. Maybe you could name one or several widely-known PDEs and physical areas as examples right in the introduction (e.g. Navier-Stokes for fluid dynamcis?)

Thank you for your comment, @flokno. I have elaborated a little in the Statement of need to include some examples of famous PDEs.

Thank you @vidarsko . I find "Maxwell's equations which dictate how electromagnetic fields encode those well-crafted cat memes" a little bit too colloquial and confusing, but that's just a comment.

I have found some problems when running the tutorials locally and not in the cloud, please see vidarsko/ComFiT#13

Thank you for your thorough review, @flokno. I suggested a solution to your raised issue. Have a look and see if it is to your satisfaction.

I have also revised the sentece which, I agree, was perhaps a bit too colloquial.

flokno commented 4 months ago

@vidarsko I tried the other tutorials and ran into a bunch of errors, see issues. I stopped for now -- please double-check all tutorials and let me know when they are ready. Once all tutorials works, I'm fine with the JOSS paper!

As an optional comment: there are tools to auto-format python files and jupyter notebooks. For example: https://github.com/drillan/jupyter-black

I strongly recommend consistent code formatting complying with PEP8 (and possibly tools like flake8 to check), though this is of course optional

jonasron commented 4 months ago

@vidarsko I tried the other tutorials and ran into a bunch of errors, see issues. I stopped for now -- please double-check all tutorials and let me know when they are ready. Once all tutorials works, I'm fine with the JOSS paper!

As an optional comment: there are tools to auto-format python files and jupyter notebooks. For example: https://github.com/drillan/jupyter-black

I strongly recommend consistent code formatting complying with PEP8 (and possibly tools like flake8 to check), though this is of course optional

@flokno Thank you for going over the tutorials. We have checked all tutorials and they are now working.

flokno commented 4 months ago

@jonasron @vidarsko good, thank you! I ticked the last box.

@lucydot I'm done with my review and I think the package is good to go!

vidarsko commented 4 months ago

Thank you! @lucydot, what is the next step?

lucydot commented 4 months ago

Excellent work @flokno @sharanroongta @sbacchio; on behalf of the JOSS team, thank you for your expertise and time.

lucydot commented 4 months ago

We are onto the final stages of the process now @vidarsko I'll ask editorialbot to generate the post-review to-do list; let me know when each item is done.

lucydot commented 4 months ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

lucydot commented 4 months ago

@editorialbot generate pdf

lucydot commented 4 months ago

@editorialbot check references

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:

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

OK DOIs

- 10.1103/PhysRevLett.78.401 is OK
- 10.1103/PhysRevE.59.1574 is OK
- 10.1103/PhysRevE.85.011153 is OK
- 10.1006/jcph.2002.6995 is OK
- 10.1103/PhysRevE.95.052144 is OK
- 10.1103/PhysRevLett.121.255501 is OK
- 10.1103/PhysRevE.93.042137 is OK
- 10.1103/PhysRevE.93.032106 is OK
- 10.1103/PhysRevB.103.014107 is OK
- 10.1103/PhysRevB.103.224107 is OK
- 10.1016/j.jmps.2022.104932 is OK
- 10.1088/1361-651X/ac9493 is OK
- 10.1038/s41524-023-01077-6 is OK
- 10.1039/D3SM00316G is OK
- 10.1088/1367-2630/ab95de is OK
- 10.1103/PhysRevResearch.5.023108 is OK
- 10.11588/ans.2015.100.20553 is OK
- 10.25561/104839 is OK
- 10.1103/PhysRevResearch.2.023068 is OK

MISSING DOIs

- Errored finding suggestions for "A Unified Perspective on Two-Dimensional Quantum T...", please try later
- Errored finding suggestions for "Symmetry, Topology, and Crystal Deformations: A Ph...", please try later
- Errored finding suggestions for "Topological Defects and Flows in BECs and Active M...", please try later
- Errored finding suggestions for "PyClaw: Accessible, Extensible, Scalable Tools for...", please try later

INVALID DOIs

- None
lucydot commented 4 months ago

@vidarsko, some comments on the paper:

Once you have updated the paper, please run @editorialbot generate pdf to double check that the paper compiles and reads as expected.

Nearly there!

lucydot commented 4 months ago

@vidarsko for the cited doctoral theses, please can you double check if there are DOIs associated. Usually there is an institutional record with an assigned DOI. Thanks.