Closed editorialbot closed 9 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
Software report:
github.com/AlDanial/cloc v 1.88 T=0.04 s (1046.4 files/s, 93583.6 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Python 35 508 691 1900
Markdown 2 106 0 251
YAML 5 24 19 149
TOML 1 16 0 147
TeX 1 8 0 116
-------------------------------------------------------------------------------
SUM: 44 662 710 2563
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repository
Wordcount for paper.md
is 1095
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1063/1.871116 is OK
- 10.1006/jcph.1997.5697 is OK
- 10.1088/1361-6587/aaa373 is OK
- 10.1088/1361-6587/ace993 is OK
- 10.1063/1.5122865 is OK
- 10.1088/1361-6587/abad02 is OK
- 10.1063/1.871566 is OK
MISSING DOIs
- None
INVALID DOIs
- None
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
π @the-rccg @archermarx @Uddiptaatwork the review will take place here
I have completed most of my review, except for actually running the software. Overall, I think this is a very nice contribution. I have two main comments:
You do a good job of documenting the available methods, but you are missing an example or two to demonstrate how to use the code. I would include a small tutorial in the documentation showing how to use the code and showing off a few configurable options.
On a similar point, you mention in the installation instructions that you can use accelerators like numba. It would be good to have a list of which ones are supported by the code.
I will try and run the software tomorrow.
@archermarx I added an example for the CLI interface with an explanation for the kwargs used. There is also an additional paragraph on the accelerators that clearly labels available accelerators (currently only Numba). Others should be ready soon, and will be added to the readme and installation. For a full workflow example, a more thorough framework would be needed like a Google Colab Notebook, which is planned for the future.
Great! I've now run the code and it mostly works. However, I encounter an error using the following input
python3 -m hw2d --step_size=0.025 --end_time=100 --grid_pts=128 --c1=1.0 --k0=0.15 --N=3 --nu=5.0e-08 --snaps=1 --buffer_size=100 --output_path="test.h5" --movie=1 --min_fps=10 --speed=5 --debug=0
The code runs to completion and generates a (very nice looking) movie. However, in the "Plotting properties" step, I get this error:
100%|βββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββ| 41/41 [00:16<00:00, 2.46it/s]
Plotting properties...ββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββ | 40/41 [00:16<00:00, 2.53it/s]
/home/marksta/.local/lib/python3.10/site-packages/numpy/core/_methods.py:206: RuntimeWarning: Degrees of freedom <= 0 for slice
ret = _var(a, axis=axis, dtype=dtype, out=out, ddof=ddof,
/home/marksta/.local/lib/python3.10/site-packages/numpy/core/_methods.py:163: RuntimeWarning: invalid value encountered in divide
arrmean = um.true_divide(arrmean, div, out=arrmean,
/home/marksta/.local/lib/python3.10/site-packages/numpy/core/_methods.py:198: RuntimeWarning: invalid value encountered in divide
ret = ret.dtype.type(ret / rcount)
/home/marksta/.local/lib/python3.10/site-packages/numpy/core/fromnumeric.py:3504: RuntimeWarning: Mean of empty slice.
return _methods._mean(a, axis=axis, dtype=dtype,
/home/marksta/.local/lib/python3.10/site-packages/numpy/core/_methods.py:129: RuntimeWarning: invalid value encountered in divide
ret = ret.dtype.type(ret / rcount)
test_enstrophy-energy-kinetic_energy-thermal_energy.jpg
202it [02:02, 1.65it/s]
And no image is produced.
@archermarx I updated the error handling to give graceful explanations and handling of this case. The issue was that end_time=100 is not inside of the saturated turbulent phase, so all statistical properties would be meaningless for the turbulent system. It now provides a warning and includes a picture of the non-converged data. The start of the different phases are given in the section subtitled "Dynamics of the different phases" in the readme.
Excellent! In that case, I think I can mark my review as completed. Very cool work!
@archermarx Thank you very much for your time and effort in finding these issues and reviewing the submission! Really appreciate it.
I have now concluded my review. The code works great and the tests ran smoothly without any errors. Here is one point that the author might consider:
The documentation pertaining to the API is very well done and appears highly useful for any new user of the code. The article is succinct with analytical descriptions and citations where necessary.
Overall, I would like to congratulate the author for a very well written code and article. I can mark my review as completed.
Thank you, @Uddiptaatwork.
@the-rccg, is that suggestion on a home page for the documentation something you could address?
@Uddiptaatwork Thank you very much for the review. I have added a dynamic link for the top-level that includes the README.md dynamically in documentation creation upon commit. pdoc3 currently has some issues with latex formulas in include and does no support for GitHub Flavored Markdown (GFM), therefore some things are not properly displayed at this time. I have opened an issue with pdoc3 and will try to get the support implemented myself to make sure it displays properly. For now, it's a crude approximation of the readme and should be fixed soon with progress on the pdoc3.
Preview of the ReadMe in the docs: https://the-rccg.github.io/hw2d/index.html
@the-rccg in the past, I think I converted my Markdown README to ReST and then used that for the docs generation to avoid some issues like that, but may have been with Sphinx. This looks like a good solution, though
@kyleniemeyer After a lot of back and forth, I have now moved from pdoc3 to pdoc. Images and equations now work and all information is pulled dynamically from the readme.
That looks great! At this point, I think the review is complete.
@editorialbot set <DOI here> as archive
@editorialbot set <version here> as version
@editorialbot generate pdf
@editorialbot check references
and ask author(s) to update as needed@editorialbot recommend-accept
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Hi @the-rccg, above you can see a checklist of items to complete as we wrap up the review. Can you work through your items?
Additional Author Tasks After Review is Complete:
Release: v1.0.0 (Official JOSS release)
ORCID from Zenodo: https://zenodo.org/doi/10.5281/zenodo.10365012
@the-rccg I made a few edits to the paper, can you merge those? https://github.com/the-rccg/hw2d/pull/4
Also, can you add the DOI to the Goumiri et al. reference? 10.1063/1.4796190
@editorialbot set 10.5281/zenodo.10365012 as archive
Done! archive is now 10.5281/zenodo.10365012
@editorialbot 1.0.0 as version
I'm sorry human, I don't understand that. You can see what commands I support by typing:
@editorialbot commands
@editorialbot set 1.0.0 as version
Done! version is now 1.0.0
@kyleniemeyer merged and added all missing DOIs
@editorialbot generate pdf
@editorialbot check references
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1063/1.871116 is OK
- 10.1006/jcph.1997.5697 is OK
- 10.1088/1361-6587/aaa373 is OK
- 10.1088/1361-6587/ace993 is OK
- 10.1063/1.5122865 is OK
- 10.1103/PhysRevLett.50.682 is OK
- 10.1063/1.4796190 is OK
- 10.1088/1361-6587/abad02 is OK
- 10.1063/1.871566 is OK
MISSING DOIs
- None
INVALID DOIs
- None
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
OK, all looks great @the-rccg!
@editorialbot recommend-accept
Attempting dry run of processing paper acceptance...
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1063/1.871116 is OK
- 10.1006/jcph.1997.5697 is OK
- 10.1088/1361-6587/aaa373 is OK
- 10.1088/1361-6587/ace993 is OK
- 10.1063/1.5122865 is OK
- 10.1103/PhysRevLett.50.682 is OK
- 10.1063/1.4796190 is OK
- 10.1088/1361-6587/abad02 is OK
- 10.1063/1.871566 is OK
MISSING DOIs
- None
INVALID DOIs
- None
The paper's PDF and metadata files generation produced some warnings that could prevent the final paper from being published. Please fix them before the end of the review process.
athrm{d}^2 x \space \left(n \partial_y \
^
unexpected control sequence \space
expecting "%", "\\label", "\\tag", "\\nonumber" or whitespace
athrm{d}^2 x \space \left(n - \phi\right
^
unexpected control sequence \space
expecting "%", "\\label", "\\tag", "\\nonumber" or whitespace
= \small \frac{1}{2} \normal
^
unexpected control sequence \small
expecting "%", "\\label", "\\tag", "\\nonumber" or whitespace
= \small \frac{1}{2} \normal
^
unexpected control sequence \small
expecting "%", "\\label", "\\tag", "\\nonumber" or whitespace
mathrm{d} k_y \space \Gamma^n \small (k_
^
unexpected control sequence \space
expecting "%", "\\label", "\\tag", "\\nonumber" or whitespace
\delta \small (k_y) \normalsize \spac
^
unexpected control sequence \small
expecting "%", "\\label", "\\tag", "\\nonumber" or whitespace
E^N \small (k_y) \normalsize \space
^
unexpected control sequence \small
expecting "%", "\\label", "\\tag", "\\nonumber" or whitespace
E^V \small (k_y) \normalsize \space
^
unexpected control sequence \small
expecting "%", "\\label", "\\tag", "\\nonumber" or whitespace
\mathrm{d}^2 x \space (n \mathfrak{D^n}
^
unexpected control sequence \space
expecting "%", "\\label", "\\tag", "\\nonumber" or whitespace
\mathrm{d}^2 x \space (n - \Omega)(\mat
^
unexpected control sequence \space
expecting "%", "\\label", "\\tag", "\\nonumber" or whitespace
\mathfrak{D}^n \small (x,y) \normalsize
^
unexpected control sequence \small
expecting "%", "\\label", "\\tag", "\\nonumber" or whitespace
:wave: @openjournals/pe-eics, this paper is ready to be accepted and published.
Check final proof :point_right::page_facing_up: Download article
If the paper PDF and the deposit XML files look good in https://github.com/openjournals/joss-papers/pull/4832, then you can now move forward with accepting the submission by compiling again with the command @editorialbot accept
Can the Tex warnings be safely ignored or do I need to removethe \space and \small from it still?
@the-rccg let's try removing \space
and \small
in the equations. If you need spacing in math mode, I believe you can use \,
, \:
, and \;
instead.
As for \small
, I'm not sure the output you were aiming for there. Is that needed?
@kyleniemeyer removed all \small, \normalsize, and \space and replaces some with more explicit spacings.
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@editorialbot recommend-accept
Submitting author: !--author-handle-->@the-rccg<!--end-author-handle-- (Robin Greif) Repository: https://github.com/the-rccg/hw2d Branch with paper.md (empty if default branch): main Version: 1.0.0 Editor: !--editor-->@kyleniemeyer<!--end-editor-- Reviewers: @archermarx, @Uddiptaatwork Archive: 10.5281/zenodo.10365012
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
@archermarx & @Uddiptaatwork, 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 @kyleniemeyer 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 @archermarx
π Checklist for @Uddiptaatwork