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
[REVIEW]: PythonicDISORT: A Python reimplementation of the Discrete Ordinate Radiative Transfer package DISORT #6442
Comments
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:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
|
Software report:
Commit count by author:
|
Paper file info: 📄 Wordcount for ✅ The paper includes a |
License info: ✅ License found: |
@dhjx1996 as reported above by editorialbot, there are two references with invalid DOIs. When you have a moment, please fix these, thanks |
Review checklist for @arjunsavelConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
I have fixed these and pushed the changes. |
@editorialbot check references |
|
Review checklist for @simonrp84Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
@arjunsavel, @simonrp84 & @pscicluna, thanks again for reviewing! Just checking in on your review. Please let me know if you have any questions about the process. Feel free to create issues in project repository directly or write them down as comments here, but please do link the issues in this review so it's easy to follow for everyone. @pscicluna, please go ahead and create your checklist first using the command |
@arjunsavel, @simonrp84 & @pscicluna let me know if there is anything stopping you from starting your reviews. We aim for a publication time of ~3 months, so it is ideal if reviewers can complete their review in 4-6 weeks. |
Thank you for your patience. I’ve finished my review of the PythonicDISORT package. It’s an excellent piece of work that has clear utility in both pedagogical and research contexts. My comments are below. Provided they’re addressed, I recommend this project for acceptance. Miscellaneous
Code edits
Paper edits
Documentation
|
Thank you @arjunsavel for the update on your review! @dhjx1996, feel free to get started working on the comments and issues raised by the reviewer. @simonrp84 & @pscicluna please let me know if you have any questions or if you need more time for your reviews, thanks. |
@arjunsavel Thank you very much for the review. I greatly appreciate the feedback and praise. Here are my updates with respect to your feedback:
The repository's large size is due to the .npz files for the pytest reference solutions. I have reduced the
Made changes to README.md.
Added.
It is not a priority especially since the feature is not in DISORT, but as a rule I am happy to add features to PythonicDISORT.
I have improved the documentation in every module.
This is caused by the length of the NT correction functions. I can modularize the NT corrections but I am currently disinclined to that change since it would be a major deviation from DISORT wherein NT corrections are under-the-hood and always performed if possible.
Yes, and I have and will continue to optimize the code. As of v0.7.0, however, I believe every expensive operation (e.g. diagonalization) is vectorized and no longer in Python for-loops.
Added.
Added.
No. I added clarifications.
Speed optimization has been and remains a goal, but it is impossible for the Python-based PythonicDISORT to match the FORTRAN-based DISORT in speed. I have attempted parallelization with joblib but the overhead actually results in a net slow down for all practical use cases. I am disinclined to use numba or Jax because they diminish the main advantage of PythonicDISORT which is its ease of use and installation. In fact, I have a guideline in CONTRIBUTING.md stating that "PythonicDISORT should remain system agnostic and installable through a simple
These changes made been made.
Unfortunately I am not aware of any published applications at the moment (PythonicDISORT is quite new). At least two of the projects which are using PythonicDISORT are close to publication, however, and if they are published before this then I will add the citations.
They are Python wrappers around DISORT. I have added two citations into the paper.
Added. |
@editorialbot check references |
|
@editorialbot generate pdf |
@phibeck @simonrp84 @arjunsavel @pscicluna The latest version of PythonicDISORT is now v0.7.0. This has been released on GitHub and on PyPI. |
@phibeck Regarding the missing DOIs, these are all references to packages. The one exception is the book "Radiative Transfer" by Chandrasekhar (1960) for which I could not find a DOI. |
Apologies for the delay in reviewing - I have been more busy than expected recently. I will submit my review later this week. |
This is a very interesting library, thanks for creating it! Unfortunately I cannot test all of the code due to a missing module, for which I've created a github issue. This module is included in the github repo, but the docs do not describe how to install it. I agree with @arjunsavel's comments on the manuscript and library, so please address all of those. In addition, I have some other points to raise - which are listed below. Paper comments:
Code/repo comments:
I've ticked the appropriate boxes in the reviewer checklist, and will await an author response before going further. Sorry again for my long delay in performing this review. |
@simonrp84 Thank you very much for the review! I will address the most urgent matters and follow-up on the rest later this week.
I see two possibilities going forward:
I am currently inclined towards 2) as it would also reduce the size of the repository, but feedback on this would be appreciated.
Minor remark: I have already cleaned the history using |
As a follow-up to my previous comment, I will revamp the current test notebooks into examples for users and remove |
Hi @dhjx1996 I think this is a good idea to avoid licensing issues, although the details of such potential issues would probably depend on the license of DISORT. It seems sensible to me to remove the module and just provide the reference solutions as benchmarks. Of course that requires that the author(s) of PythonicDISORT keep this data up-to-date. Thanks @simonrp84 for getting your review started! @pscicluna please let me know if you have any question about the review process or if you need more time as ideally we'd like to finish the review within 4-6 weeks. |
Submitting author: @dhjx1996 (Jia Xu Dion Ho)
Repository: https://github.com/LDEO-CREW/Pythonic-DISORT
Branch with paper.md (empty if default branch):
Version: v0.4.2
Editor: @phibeck
Reviewers: @arjunsavel, @simonrp84, @pscicluna
Archive: Pending
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
@arjunsavel & @simonrp84 & @pscicluna, 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 @phibeck 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 @arjunsavel
📝 Checklist for @simonrp84
The text was updated successfully, but these errors were encountered: