-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PyPartMC #179
Comments
Hey @slayoo, I'm Alex, currently serving as the Editor-in-Chief. I'm sorry it took me so long to get back to you.
Please find below the preliminary checks. Editor-in-Chief checksThank you for submitting your package for pyOpenSci review. Below are the basic checks that your package needs to pass to begin our review. If some of these are missing, we will ask you to work on them before the review process begins. Please check our Python packaging guide for more information on the elements below.
Editor commentsI have a few suggestions regarding the documentation:
|
@Batalex, thanks for the feedback! |
@Batalex, with the following PRs just merged:
the repo and docs are improved following your suggestions: |
Thank you, @slayoo, the docs look great! |
thank you! |
Sorry, I forgot to add something important to my previous message. Since our field of expertise is Python, the significant portion of C++ code in |
Hello @slayoo, thank you for patience! I'm Chiara, I am following up as Editor in chief. |
Thank you, @russbiggs and @cmarmo! |
@slayoo Sorry for the slow start on my part as editor, I am currently seeking reviewers and will update as that progresses. |
hey team - i'm checking in on this review. I think we need an editor for it. Is that correct? If so i can put this one on my radar to find someone to edit. |
thank you @lwasser |
Hello @slayoo, I'm going to take care of PyPartMC as editor. |
thank you @cmarmo |
Hi @slayoo, I am glad to announce that we have our first reviewer! 🥳 I am actively looking for a second reviewer, but I think Simon can start to review, as this submission already waited a bit. Before beginning your review, please fill out our pre-review survey. This helps us improve all aspects of our review and better understand our community. No personal data will be shared from this survey - it will only be used in an aggregated format by our Executive Director to improve our processes and programs. The following resources will help you complete your review:
|
Hello @slayoo, @cmarmo commented on the scientific-python discord, asking for reviewers. I'm happy to review this if no one else becomes available soon. I've done significant work using C++/Fortran + Python. However, I haven't done much with pybind11 or CMake. Happy to put some work in and brush up on those. Before committing, I’ve signed up to a couple of different things that I’ll need to prioritise. It may take me a while (at the latest, over the Christmas break) to get a quality review done. Is all that acceptable for you? Happy to be superseded if someone else becomes available. |
@Kai-Striega thank you so much for offering your help! |
Dear @slayoo. I am struggling with
final lines:
|
@simonom, thank you! So far, we have not yet produced binary wheels for Python 3.13, please stay tuned, hope to roll these out in the coming days at least for macOS. Since there are no binary wheels available for your platform/OS setup, pip tries to compile the package from source. The failure in getting it compiled seems to originate from an incompatibility around the Thanks! |
@Kai-Striega, of course, thank you! |
Hi @slayoo, I am glad to announce that we have our second reviewer! 🥳 Before beginning your review, please fill out our pre-review survey. This helps us improve all aspects of our review and better understand our community. No personal data will be shared from this survey - it will only be used in an aggregated format by our Executive Director to improve our processes and programs. The following resources will help you complete your review:
Now that we have the two reviewers assigned I am tentatively setting a deadline for the review of PyPartMC: let's try to give a nice Christmas gift to PyPartMC maintainers and let the review in for December 23th! 🎄 Thank you all for your work and your patience! |
@simonom I took the liberty to highlight the traceback in your previous comment. I see you are using a miniconda installation, in order to move forward with the review, may I suggest to create a conda/mamba environment with a 3.12 python version? Also @dbaston, @simonom , please let me know if the review deadline is realistic. Thank you! 🙏 |
@cmarmo, yes, I will continue with a compatible python version, and yes, deadline can be met! |
As of PyPartMC v1.3.10 (released earlier today), there are binary wheels on PyPI for Python 3.13 on macOS. Supported OS versions are: macosx-13 (universal binary) and macosx-14 (ARM binary). Hope it helps! Feedback welcome. Note that Python 3.13 is not fully supported in auxiliary packages used in PyPartMC example notebooks, notably:
|
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
I did not find instructions for installing the development version. After pulling the submodules, running
I ran the notebooks on Google Colab without issue. It would be helpful to have instructions for running the notebooks locally, as additional dependencies such as
While the documentation contains an entry for user-facing functions, it isn't always clear what the expected arguments or return values are. Some argument names are not rendered in the documentation. For example,
I did not find examples in the Python API documentation.
It would be helpful to describe how someone can contribute to the documentation. This may be as simple as describing where the docstring text can be found.
Included in Readme file requirements
The README should include, from top to bottom:
NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)
None required.
Described in the linked paper.
UsabilityReviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole.
Functionality
Did not evaluate.
Did not evaluate.
Did not see instructions for running tests. Tests run successfully using
Did not evaluate. A code coverage report of the C++ binding code would be useful.
PyPartMC uses a custom It wasn't clear to me why C++ class methods were implemted as static methods manually taking a reference to
For packages also submitting to JOSS
Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted. The package contains a
Final approval (post-review)
Estimated hours spent reviewing: 6 Review CommentsThis package cleanly exposes Fortan code to Python using pybind11. I did not see any serious code quality issues. Tests run smoothly but I was not able to easily determine which code is covered by the tests. HDF and netCDF being fairly common libraries, I wonder about the potential for runtime issues if the version vendored by PyPartMC differs from a version loaded by another package such as |
Thank you @dbaston! Your work is much appreciated and helpful. |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
The authors could be more explicit in the README introduction, such as providing an example of the user you are serving. Actually, the goals stated in the ‘Target audience and scientific applications’ sections of the review page (PyPartMC · Issue #179 · pyOpenSci/software-submission) seem very suitable for this section.
The user could benefit from some description around the pip install PyPartMC command to explain that CMake and a fortran compiler are required. This is well detailed in the FAQs, but it makes sense for these dependencies to be stated before the pip install instruction for PyPartMC, including the necessary versions (e.g. python).
The API documentation is very valuable for explaining how a user can set inputs for functions, but the link is somewhat hidden in the Features section of README. I suggest a more obvious presence in README.
In CITATION.cff is author information in addition to a doi to an associated published paper. In Credits of README is funding information. Readme file requirements
The README should include, from top to bottom:
Would be useful to have a repostatus badge. NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)
The authors could be more explicit in the README introduction, such as providing an example of the user you are serving. Actually, the goals stated in the ‘Target audience and scientific applications’ sections of the review page (PyPartMC · Issue #179 · pyOpenSci/software-submission) seem very suitable for this section.
Because the README does not contain a section titled ‘Installation’ it is really easy to miss the ‘pip install PyPartMC’ command, so recommend making this clearer. Preferably this installations section would also describe the dependencies on Fortran and CMake.
Not applicable for local install, and additional setup for vignettes is well described
I think the vignettes are a real strong point of the repository.
The python and Julia example are provided. Like with the Short description of package goals (for which I recommend an example of a target user), it would help to have more detail in the ‘Usage examples’ introduction about why a user would be interested in randomly sampling particles from an aerosol size distribution. I think this is particularly important because PyPartMC is aiming to draw in more novice users of PartMC, so these people will potentially be new to PartMC.
As stated by me in the Function Documentation: section above, it would be useful to have the link to the API docs more visible than the brief mention in the Features section.
I think this section needs including in the README, particularly as the package is an interface to another package (PartMC). This section would be an opportunity to really clearly explain what the implications of using this package are over using the PartMC package directly. It would also be good to include in this section the references included in ‘Other Python packages with relevant feature scope’ in the review page (PyPartMC · Issue #179 · pyOpenSci/software-submission)
I think this section needs including in the README, particularly as the package is an interface to another package (PartMC). This section would be an opportunity to really clearly explain what the implications of using this package are over using the PartMC package directly. It would also be good to include in this section the references included in ‘Other Python packages with relevant feature scope’ in the review page (PyPartMC · Issue #179 · pyOpenSci/software-submission) UsabilityReviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole.
See notes above on clearer linking to the API Docs in README
See notes above on clearer linking to the API Docs in README
The API Docs are impressive, and some examples are also provided
Functionality
Some tests need pytest installed, so this should be stated along with other install depenencies
I can see that CI is setup in the .github/workflows folder, but note that the link in the ‘CI builds’ text in ‘Features’ of README is broken
For packages also submitting to JOSS
Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted. The package contains a
Final approval (post-review)
Estimated hours spent reviewing: 7 Review CommentsI've enjoyed reviewing this package, which has been very well put together overall. The comments I have provided I believe cover just minor changes. |
Submitting Author: Sylwester Arabas (@slayoo)
All current maintainers: (@zdaq12, @jcurtis2, @nriemer, @mwest1066)
Package Name: PyPartMC
One-Line Description of Package: Python interface to PartMC aerosol-dynamics Monte-Carlo simulation package
Repository Link: https://github.com/open-atmos/PyPartMC/
Version submitted: v1.2.0
EIC: @Batalex
Editor: @cmarmo
Reviewer 1: @simonom
Reviewer 2: @dbaston
Archive: TBD
Version accepted: TBD
Date accepted (month/day/year): TBD
Code of Conduct & Commitment to Maintain Package
Description
PyPartMC is a Python interface to PartMC, a particle-resolved Monte-Carlo code for atmospheric aerosol simulation. PyPartMC is implemented mostly in C++ (based on the pybind11 framework) with some C and Fortran boilerplate layers. PyPartMC constitutes an API to the PartMC Fortran internals. Besides empowering Python/Jupyter users, PyPartMC can facilitate using PartMC from other environments - see, e.g., Julia and Matlab examples in the project README.
Scope
Domain Specific
n/a
(atmospheric science)
Community Partnerships
n/a
(we host development as a part of the OpenAtmos initiative: https://github.com/open-atmos)
Target audience and scientific applications
Development of PyPartMC has been intended to remove limitations to the use of Fortran-implemented PartMC software. PyPartMC facilitates the dissemination of computational research results by streamlining independent execution of PartMC simulations, which could prove advantageous during peer review process. Additionally, the ability to easily package examples, simple simulations, and results in a web-based notebook allows PyPartMC to support the efforts of many members of the scientific community, including researchers, instructors, and students, with nominal software and hardware requirements.
Other Python packages with relevant feature scope
Technical checks
For details about the pyOpenSci packaging requirements, see our [packaging guide][PackagingGuide]. Confirm each of the following by checking the box. This package:
Publication Options
(we have recently published a SoftwareX paper on PyPartMC: https://doi.org/10.1016/j.softx.2023.101613)
Are you OK with Reviewers Submitting Issues and/or pull requests to your Repo Directly?
Confirm each of the following by checking the box.
Please fill out our survey
The text was updated successfully, but these errors were encountered: