Closed whedon closed 2 years ago
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @jwuttke, @darothen it looks like you're currently assigned to review this paper :tada:.
:warning: JOSS reduced service mode :warning:
Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.
:star: Important :star:
If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿
To fix this do the following two things:
For a list of things I can do to help you, just type:
@whedon commands
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@whedon generate pdf
PDF failed to compile for issue #3219 with the following error:
Can't find any papers to compile :-(
Software report (experimental):
github.com/AlDanial/cloc v 1.88 T=0.41 s (534.1 files/s, 31905.4 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Python 212 2053 1184 7548
SVG 1 0 19 1402
Jupyter Notebook 3 0 207 336
Markdown 1 38 0 280
YAML 4 20 4 112
-------------------------------------------------------------------------------
SUM: 221 2111 1414 9678
-------------------------------------------------------------------------------
Statistical information for the repository 'ee5fa7f3802655beaf36d792' was
gathered on 2021/04/26.
The following historical commit information, by author, was found:
Author Commits Insertions Deletions % of changes
Anna Jaruga 1 3 1 0.01
Bartosz Piasecki 1 42 23 0.08
Michael 18 443 170 0.80
Michaeldz36 37 2661 2424 6.64
Piotr Bartman 10 14 19 0.04
Sylwester Arabas 417 11939 7609 25.54
piotrbartman 61 2072 2853 6.44
prbartman 438 26379 19740 60.27
tehAgitto 4 84 51 0.18
Below are the number of rows from each author that have survived and are still
intact in the current revision:
Author Rows Stability Age % in comments
Bartosz Piasecki 30 71.4 5.1 0.00
Michael 289 65.2 15.1 10.03
Sylwester Arabas 4438 37.2 3.2 4.35
piotrbartman 6011 290.1 9.3 7.97
tehAgitto 42 50.0 4.5 2.38
@whedon generate pdf from branch JOSS
Attempting PDF compilation from custom branch JOSS. Reticulating splines etc...
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
PySDM
is a new open-source package for particle-resolved modeling of the physics and chemistry of cloud and aerosol microphysics. It provides high-performance reference implementations of state-of-the-science process models in a flexible, highly-configurable package that is relatively easy to manipulate and modify to suit a wide variety of cloud and aerosol modeling tasks. It also ships with a back-end for running certain processes on GPUs, although this functionality does not seem to be fully implemented for all configurations which a user may seek to leverage.
Overall this is a high quality and valuable contribution to the field. Many researchers leverage simple box and parcel models for critical work into fundamental aerosol/cloud microphysics, and PySDM
provides extremely useful tooling for exploring this domain and for replicating the results of other researchers. However, the library is lacking in documentation, both formally (in clearly defining the complex structure of the objects which comprise the code and how they interact) and informally (through docstrings and other comments/annotations in the code which may allow users to better understand arguments to key functions and how to manipulate and extend them). At present, the authors provide a nice "Hello world" example with some prose on how to configure it, as well as a basic section on package structure and API. But a more comprehensive listing of the arguments, attributes, and parameters that the user should know about in order to configure or extend this example would go a very long way to promote practical usability.
I would encourage the authors to improve or augment this documentation before the final publication of this software paper. However, in all other aspects I find this to be a comprehensive and uniquely valuable software contribution to the cloud and aerosol microphysical modeling community and an ideal candidate for publication in JOSS.
Please note the handful of detailed comments below on specific changes necessary to meet JOSS standards.
A simple install recipe using PyPI is provided, but given the capability to run on the GPU a conda-forge
recipe or something which can help readers understand requirements - or provide a turnkey installation with necessary dependencies - would be very helpful! (Issue 494)
Installation using the documented instructions misses out on installing pystrict
, a required dependency (Issue/PR 495)
The automated tests seem to work out of the box, but the smoke tests seem to take a very long time to run. Even one of the unit tests (namely attributes/chemistry/test_pH.py
) took a disproportionate amount of time compared to the others. Please note that I was not able to run the GPU tests as I did not have immediate access to a GPU-powered machine.
While there is extensive, excellent demonstration and example code available from the authors in related, clearly-linked repositories, the API documentation leaves much to be desired. There is a high degree of customizability and configuration exposed to the user, but very little in the way of curated documentation for the APIs and data structures in the software, or docstrings and annotations. As a motivating example, consider the spectra module; in this module, a nice base class for building probability distributions is exposed, but there are no docstrings describing the required and optional arguments for either the base class or any of the sub-classes which implement it. An effort to improve the documentation with basic docstrings for all the core methods and functionality would greatly improve the quality of the software and the user experience when adapting it for a specialized research task. Ideally, a simple Sphinx-based documentation for the key API and configurations exposed to users would follow very plainly from such an effort, greatly improving the ease of access of the software.
Community / contribution guidelines are not provided with the repo; the JOSS reviewer guidelines suggest that such documentattion should clearly indicate how third-parties may contribute to the software, report issues, and seek support should they need to do so.
The software paper does not include a section which explicitly explains a "Statement of Need." However, I do believe that the prose in the paper covers this topic with the necessary detail. I will defer to the editors whether a stand-alone "Statement of Need" section is required to meet JOSS guidelines.
It's much appreciated that the authors include links to open source software which attempts to tackle subsets of the broad functionality that PySDM
provides. I would strongly encourage the author to include a brief discussion of each package (particularly pyrcel
and PyBox
) and compare/contrast the unique or improved functionalities of PySDM
relative to each. My take (as the author of one of those libraries, pyrcel
!) is that PySDM
offers a substantial improvement over each in terms of the breadth of calculations and science/research that can be performed with it as well as the cleanliness of its interface and design for future improvements and extensions. These are all very positive things for PySDM
and I would strongly encourage the authors to lean into the fantastic work here to promote the capabilities and utility of their library relative to the competition!
@darothen, on behalf of all authors, let me thank for the feedback!
Let me just address one comment right away:
Two authors on the paper (Grzegorz Lazarski and Aleksandra Talar) do not seem to appear as contributors to the repo and aren't listed in "Author Contributions." I don't really believe there is any issue here - for long-incubated software projects like this with lots of research interfaces there have to be many contributors over time! But I do want to note this for the editors.
GŁ's (@golui) and AT's (@aleksandratal) contributions were subject to refactors during which we've not preserved commit authorship, yet these can be traced back, e.g., here: https://github.com/atmos-cloud-sim-uj/PySDM/pull/157 & https://github.com/atmos-cloud-sim-uj/PySDM/pull/142 Both are mentioned in the "Author Contributions" section:
Thanks!
@whedon generate pdf from branch JOSS
Attempting PDF compilation from custom branch JOSS. Reticulating splines etc...
@whedon generate pdf from branch JOSS
This was just to reflect an API change within one of the code snippets presented in the text: https://github.com/atmos-cloud-sim-uj/PySDM/commit/ca4d8214ce49abeb6b6b08892428eb04b794bc1f
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
:wave: @jwuttke, please update us on how your review is going (this is an automated reminder).
:wave: @darothen, please update us on how your review is going (this is an automated reminder).
@dhhagan Just checking in here - anything immediately needed from me here?
@slayoo please let me know if there are any PRs or other updates I've missed.
We are almost done with preparing a release and will soon after post here a point-by-point reply to the review detailing how the points were addressed in the release. Thanks, S.
@darothen Thanks for checking in -> you should be good for now. Just waiting on the other reviewer and updated/patches from the authors before any next steps are needed. Thanks for checking in!
@jwuttke Can you please update us on the status of your review? Thanks!
@whedon generate pdf from branch JOSS
Attempting PDF compilation from custom branch JOSS. Reticulating splines etc...
@whedon reming @jwuttke in 1 day
I'm sorry human, I don't understand that. You can see what commands I support by typing:
@whedon commands
@whedon remind @jwuttke in 1 day
Reminder set for @jwuttke in 1 day
:wave: @jwuttke, please update us on how your review is going (this is an automated reminder).
Hey @piotrbartman sorry about the long delays here. I am going to start looking for a new reviewer so we can finish this up. Hope to be back with some good news later this week or next.
@dhhagan Thanks for the update!
Let me provide a point-by-point reply to @darothen's review.
First of all, on behalf of all authors, let me express again our appreciation for the feedback. We have been working on addressing the points raised in the review, and a lot is already reflected in the recent 1.4 release of PySDM (https://github.com/atmos-cloud-sim-uj/PySDM/releases/tag/v1.4).
Review - @darothen
PySDM is a new open-source package for particle-resolved modeling of the physics and chemistry of cloud and aerosol microphysics. It provides high-performance reference implementations of state-of-the-science process models in a flexible, highly-configurable package that is relatively easy to manipulate and modify to suit a wide variety of cloud and aerosol modeling tasks. It also ships with a back-end for running certain processes on GPUs, although this functionality does not seem to be fully implemented for all configurations which a user may seek to leverage.
As correctly pointed out by the reviewer, at the time of the initial submission, the GPU support has been limited to the coalescence process. As of version 1.4, this is extended to all but aqueous chemistry processes, in particular supporting condensational growth/evaporation of particles (yet without adaptive timestepping which features logic less applicable in the GPU SIMT context). Parcel and single-column examples work OK on the GPU, and testing of the GPU code (through the FakeUnits mocking layer) is part of the Continuous Integration workflow.
Overall this is a high quality and valuable contribution to the field. Many researchers leverage simple box and parcel models for critical work into fundamental aerosol/cloud microphysics, and PySDM provides extremely useful tooling for exploring this domain and for replicating the results of other researchers. However, the library is lacking in documentation, both formally (in clearly defining the complex structure of the objects which comprise the code and how they interact) and informally (through docstrings and other comments/annotations in the code which may allow users to better understand arguments to key functions and how to manipulate and extend them). At present, the authors provide a nice "Hello world" example with some prose on how to configure it, as well as a basic section on package structure and API. But a more comprehensive listing of the arguments, attributes, and parameters that the user should know about in order to configure or extend this example would go a very long way to promote practical usability.
The steps taken so far to improve the documentation include:
I would encourage the authors to improve or augment this documentation before the final publication of this software paper. However, in all other aspects I find this to be a comprehensive and uniquely valuable software contribution to the cloud and aerosol microphysical modeling community and an ideal candidate for publication in JOSS.
Extending the documentation is going to be a priority for the upcoming releases and the growing user-base will help us to fulfill this commitment.
Please note the handful of detailed comments below on specific changes necessary to meet JOSS standards. General Checks
Two authors on the paper (Grzegorz Lazarski and Aleksandra Talar) do not seem to appear as contributors to the repo and aren't listed in "Author Contributions." I don't really believe there is any issue here - for long-incubated software projects like this with lots of research interfaces there have to be many contributors over time! But I do want to note this for the editors.
As pointed out above (https://github.com/openjournals/joss-reviews/issues/3219#issuecomment-827915407), contributions of both Grzegorz Łazarski and Aleksandra Talar can be traced back on github, yet the commit authorship was partly lost as a part of refactors when handling their contributions.
Functionality
A simple install recipe using PyPI is provided, but given the capability to run on the GPU a conda-forge recipe or something which can help readers understand requirements - or provide a turnkey installation with necessary dependencies - would be very helpful! (Issue 494)
We are actively working on addressing it:
Installation using the documented instructions misses out on installing pystrict, a required dependency (Issue/PR 495)
As elaborated on in the issue (https://github.com/atmos-cloud-sim-uj/PySDM/pull/495):
pystrict
was removed;setup.py
and requirements.txt
file, respectively), the requirements.txt
file was renamed to test-time-requirements.txt
;Documentation
The automated tests seem to work out of the box, but the smoke tests seem to take a very long time to run. Even one of the unit tests (namely attributes/chemistry/test_pH.py) took a disproportionate amount of time compared to the others. Please note that I was not able to run the GPU tests as I did not have immediate access to a GPU-powered machine.
The aqueous chemistry code has received a major refactor and cleanup as a part of developments leading to v1.4. The timing of test_pH.py
has improved. Part of resource usage in test_pH.py
is taken by chempy
- an external package used from within the test to compare PySDM results with an independent code.
While there is extensive, excellent demonstration and example code available from the authors in related, clearly-linked repositories, the API documentation leaves much to be desired. There is a high degree of customizability and configuration exposed to the user, but very little in the way of curated documentation for the APIs and data structures in the software, or docstrings and annotations. As a motivating example, consider the spectra module; in this module, a nice base class for building probability distributions is exposed, but there are no docstrings describing the required and optional arguments for either the base class or any of the sub-classes which implement it. An effort to improve the documentation with basic docstrings for all the core methods and functionality would greatly improve the quality of the software and the user experience when adapting it for a specialized research task. Ideally, a simple Sphinx-based documentation for the key API and configurations exposed to users would follow very plainly from such an effort, greatly improving the ease of access of the software.
Besides following the suggestion of using a Sphinx-like tool (pdoc3 in this case) and setting up automatic deployment of up-to-date API docs on Github pages as pointed above, we have also reorganised the physics-related code (which is likely to be the first a user would intend to study and extend) and grouped it in the physics
directory/subpackage with the following structure:
PySDM.physics.aqueous_chemistry
PySDM.physics.coalescence_kernels
PySDM.physics.condensation_coordinate
PySDM.physics.constants
PySDM.physics.diffusion_kinetics
PySDM.physics.diffusion_thermics
PySDM.physics.dimensional_analysis
PySDM.physics.drop_growth
PySDM.physics.formulae
PySDM.physics.hydrostatics
PySDM.physics.hygroscopicity
PySDM.physics.impl
PySDM.physics.latent_heat
PySDM.physics.particle_advection
PySDM.physics.saturation_vapour_pressure
PySDM.physics.spectra
PySDM.physics.state_variable_triplet
PySDM.physics.surface_tension
PySDM.physics.terminal_velocity
PySDM.physics.trivia
PySDM.physics.ventilation
All of the above subpackages have at least top-level docstrings included.
Community / contribution guidelines are not provided with the repo; the JOSS reviewer guidelines suggest that such documentattion should clearly indicate how third-parties may contribute to the software, report issues, and seek support should they need to do so.
A "Contributing, reporting issues, seeking support" section has been added to the README file: https://github.com/atmos-cloud-sim-uj/PySDM/pull/579
Software Paper
The software paper does not include a section which explicitly explains a "Statement of Need." However, I do believe that the prose in the paper covers this topic with the necessary detail. I will defer to the editors whether a stand-alone "Statement of Need" section is required to meet JOSS guidelines.
While the "Statement of Need" section title is not used, the Introduction section was meant to cover it.
It's much appreciated that the authors include links to open source software which attempts to tackle subsets of the broad functionality that PySDM provides. I would strongly encourage the author to include a brief discussion of each package (particularly pyrcel and PyBox) and compare/contrast the unique or improved functionalities of PySDM relative to each. My take (as the author of one of those libraries, pyrcel!) is that PySDM offers a substantial improvement over each in terms of the breadth of calculations and science/research that can be performed with it as well as the cleanliness of its interface and design for future improvements and extensions. These are all very positive things for PySDM and I would strongly encourage the authors to lean into the fantastic work here to promote the capabilities and utility of their library relative to the competition!
While wholeheartedly agreeing that a comparison of features of different parcel models available in the Python ecosystem would be of great value for the community, we argue that it falls out of scope of the PySDM JOSS paper for three reasons:
Last but not least, let me note and ask for confirmation that we have added Oleksii Bulenok @abulenok to the paper author list. Oleksii has worked on the export mechanisms in PySDM and his VTK exporter PR has been merged before the v1.4 release offering the PySDM users to smoothly save and view multi-dimensional simulation state in such tools as Paraview (now mentioned in the text).
For the record, PySDM v1.4 is now available on PyPI as well: https://pypi.org/project/PySDM/
👋 @andreas-h, @josephhardinee, @ashwinvis : are you available and interested in reviewing this paper for JOSS?
I could probably help generically on the python and modeling side, but let me check if a colleague with more domain knowledge, especially around chemistry, would be interested in reviewing for JOSS first.
@dhhagan One is overbooked right now and the other has a COI so I can act as a second reviewer for this.
@josephhardinee Thanks! I will edit the checklist above and invite you.
@whedon add @josephhardinee as reviewer
OK, @josephhardinee is now a reviewer
Hi @dhhagan, can you advise on next steps here? I want to keep this on my radar.
Hey @darothen thanks for checking in! As of now, we're just waiting on the review from @josephhardinee. I believe the authors made changes in response to your initial comments, so you can either review and update your checklist now or wait until the final revision comes in. Everything is a bit slow right now during the pandemic, but thanks for being on top of it!
Thanks @dhhagan, I'll wait until the final revision.
Hi @josephhardinee - just wanted to ping you one more time to see if you still have this on your radar before moving on. Please let me know if this review is not within your scope? Thanks!
Sorry, I accepted this then changed jobs and this fell off radar. I will get this done this week.
No problem, @josephhardinee - thanks so much for the quick reply!
@whedon generate pdf from branch JOSS
Attempting PDF compilation from custom branch JOSS. Reticulating splines etc...
(API and affiliation updates)
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@whedon generate pdf from branch JOSS
Attempting PDF compilation from custom branch JOSS. Reticulating splines etc...
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
(added mention of one additional example based on Lowe et al. 2019 and added Clare Singer, who contributed the example, to the author list)
Hi @josephhardinee - let us ping one more time, please update us on the review, thank you for your help!
Submitting author: !--author-handle-->@piotrbartman<!--end-author-handle-- (Piotr Bartman) Repository: https://github.com/atmos-cloud-sim-uj/PySDM.git Branch with paper.md (empty if default branch): Version: v1.27 Editor: !--editor-->@dhhagan<!--end-editor-- Reviewers: @darothen, @josephhardinee Archive: 10.5281/zenodo.6321270
:warning: JOSS reduced service mode :warning:
Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.
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
@josephhardinee & @darothen, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @dhhagan 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 ✨
Review checklist for @josephhardinee
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @darothen
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper