Closed whedon closed 3 years ago
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @goldmanm, @khinsen 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
Software report (experimental):
github.com/AlDanial/cloc v 1.88 T=0.20 s (88.0 files/s, 63801.1 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Python 13 476 1122 10933
TeX 1 2 0 218
SVG 1 0 0 158
Markdown 2 43 0 81
YAML 1 1 4 18
-------------------------------------------------------------------------------
SUM: 18 522 1126 11408
-------------------------------------------------------------------------------
Statistical information for the repository '66a926c691de11eb433b2406' was
gathered on 2021/04/29.
The following historical commit information, by author, was found:
Author Commits Insertions Deletions % of changes
DrDaveShaw 31 13665 1134 100.00
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
DrDaveShaw 12531 91.7 0.1 2.78
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@DrDaveShaw. I really enjoyed testing this new package for indoor air chemistry that you wrote. I appreciate you taking the time to share it with the wider community.
A few initial questions/comments:
readme
user guide
conda install numba
even though I have anaconda installed on my computer. manuscript
model setup and use
M
in custom_input.txt
, the number density of air includes or excludes the additional VOCs being studied? INCHEM_extractor.py
file to use the updated path of the folder path from my run, and when I ran INCHEM_extractor.py
, I got an error message.
KeyError: "Passing list-likes to .loc or [] with any missing labels is no longer supported. The following labels were missing: Index(['HO2_reactivity', 'HO2_production'], dtype='object').
Changing HO2_production and HO2_reactivity to OH_production and OH_reactivity allowed for INCHEM_extractor.py to execute successfully. Was there something I did wrong when running or analyzing the code? Why was the column of data for 'HO2_reactivity' not in 'out_data.pickle'?
@goldmanm. Thank you very much for taking the time to begin a review of our package, your comments and questions are exceptionally helpful. I'll be going through them individually over the next couple of days making suggested adjustments or additions and will have a full response for you shortly.
@dhhagan After a first look at the paper and the software, I wonder what exactly JOSS expects me to do to "confirm the functional claims of the software".
The claim is basically that the program "creates and solves a system of coupled Ordinary Differential Equations (ODEs) to progress indoor atmospheric chemical species concentrations through time." Looking at the code suggests that it does indeed set up and solve a system of ODEs. But I have no idea what those equations should be, because all the scientific papers cited are behind a paywall to which I have no access. For the same reason, I cannot judge either if the numerical methods employed seem appropriate to these specific equations.
@khinsen Let me check with an EiC re: the paywalled articles.
@DrDaveShaw Are you able to provide copies of the relevant cited papers to @khinsen for the purpose of reviewing this paper?
@dhhagan We would be happy to provide relevant papers to @khinsen. As the papers are not open access we can only share them privately and we can only share papers where myself or Prof. Carslaw are authors. @khinsen would you be able to send me an email so that I can reply? (david.shaw@york.ac.uk)
@goldmanm. Thank you again for your comments and questions. I have been through them and my responses are below with references to any changes I have made to the package.
readme
Typically people often want improvements to the code to be submitted as a pull request. In the readme file, you mention having people email updates, which might not be the most efficient method of improving the code. I would recommend requesting changes be submitted as a pull request, if applicable. And if not applicable, to have the modified code be hosted on a public repository in a timely fashion.
I have changed the readme and the manual to "...we ask that you submit any improvements you make to the code to us by submitting a pull request." We do want the code to be improved by the community and this process improvement will work towards that.
user guide
Would it be possible to include source, .tex, .docx, etc, for the user manual in the repository itself so that others can adjust and build upon it if they notice any issues? You could create a separate folder in the repository for the source code.
A folder containing the source code for the user manual has been added to the repository.
The repository uses GNUv3 while the user manual uses GNUv1.3. Is it possible to synchronize these to the same license?
These refer to different licenses. The GNU General Public License for software is currently at version 3 but the manual is under the GNU Free Documentation License which is currently at version 1.3. This is as per the recommendations given on the GNU website.
It might be useful to specify a set of versions of the python packages which work with the code, in case there are future package incompatibility issues.
I have added the known working versions of the python packages to the dependencies section of the manual.
I am not sure of the statement: "If you use Anaconda, then the install includes all of the packages required and no further downloads or installs are required." I had to install and download a few of these packages with 'conda install numba' even though I have anaconda installed on my computer.
I agree that this is a potentially inaccurate and rather broad statement. Currently all packages required are included in the Anaconda 2020.11 version and the install has been tested. Given that this has historically not been the case and that previous versions of Anaconda will not have included all of the packages I have made the following adjustment to the text in the manual:
INCHEM-Py relies on a number of Python packages. If you use Anaconda, then the current version at time of writing (Anaconda 2020.11) includes all of the packages required and no further downloads or installs are required.
If you have opted to not use Anaconda, are using an older version, or you have opened a new environment without the default packages, then the following packages need to be installed:
Hopefully this is significantly clearer and now accurate.
Some description of how to implement additional chemistries into the code should be written in section 7.6, since I anticipate users might want to modify chemistry.
In section 7.6 we describe the additional indoor chemistry that has been developed and used by Carslaw and her group and which is not included in the Master Chemical Mechanism (MCM). This chemistry can be modified by users as long as the format of the module is maintained and as such I have added a description of the formatting of this module to section 7.6. However users should not add additional chemistry here, the custom_input.txt file has been included for this purpose and its use is described in section 4.3. A link to this section with an explanation has also now been added to section 7.6 for clarity.
I have also made a slight adjustment to the text in section 7.5 to describe adding surface deposition for a new species in the custom_input.txt file with an example.
manuscript
While the manuscript mentions what the software can do, it might also be useful to be more explicit about its current limitations. Is it able to resolve concentrations spatially within a room? Can it model multiple connected indoor spaces? Does any chemistry happen in or on the particle phase? Are there limitations to how wall-effects are accounted for? I can infer some of these from the user manual, but it isn't explicitly mentioned.
I have added additional text to the manuscript stating: [INCHEM-Py] "does not solve for spatial dimensions and assumes a well mixed atmosphere." I have also added further text and citations that link the methods methods for representing both the gas-to-particle partitioning for three monoterpenes and the surface deposition for relevant species, which are both discussed in detail in Carslaw 2012. Some of these methods require further reading as full explanations are beyond the scope of this paper, but we have provided enough detail for a user to decide if INCHEM-Py is suitable for their needs or can be further developed to be so.
Remove 'easily explored' from the end of the statement of need section.
Done.
model setup and use
Just for my edification, when the user sets M in custom_input.txt, the number density of air includes or excludes the additional VOCs being studied?
I assume you mean setting M in the settings.py file? M is the number density of all molecules in 1 cm3 of air and should be calculated for the temperature and pressure required.
Given that this question revealed that M could be set in the custom_input.txt file I have added a warning to try and present issues of users trying to set constants in this way.
"Be careful not to add any reactions that have already been included in the model as adding them twice will duplicate them and therefore double their rate. Any new species must have a unique name. Do not attempt to redefine any constants within this file, they can be defined in the settings.py file."
I have also added a check within INCHEM_main.py that will check the rate coefficients (the only way to define a single value in the custom_inputs.txt file) for any constants that have already been defined and ignore them with the following print statement which should prevent this issue in the future.
"x from custom_input.txt ignored. Defined elsewhere."
The default settings.py file which comes with the software tkes about 90 minutes to run. Was there any reason that this particular set of conditions was chosen to be the default that is deployed, if it is (for example, the run gives an output used in a publication), that might be useful to mention in the user guide. If it is not, do you forsee any issue with making the run shorter, so people can more quickly see if the code is functioning and analyze the output?
We agree that reducing the length of time simulated would decrease the run time and allowing users to be able to compare and check function in a shorter amount of time would be beneficial. We typically run the model for 3 days to ensure that steady-state is attained.
I have reduced the default value of "seconds_to_integrate" to 86400 which is one day and will take ~30 minutes to run. I have also updated the relevant sections of the manual.
I modified the INCHEM_extractor.py file to use the updated path of the folder path from my run, and when I ran INCHEM_extractor.py, I got an error message. Changing HO2_production and HO2_reactivity to OH_production and OH_reactivity allowed for INCHEM_extractor.py to execute successfully. Was there something I did wrong when running or analyzing the code? Why was the column of data for 'HO2_reactivity' not in 'out_data.pickle'?
This is a mistake on our part as we removed all but OH production and reactivity calculations from the default model. Instructions on how to add more species are included in the manual but HO2 reactivity and production are not calculated in the model by default. These have now been changed these to OH in the INCHEM_extractor.py script.
Let's say I am a user who downloads and runs the software according to the manual; how can I be confident that my code is working as expected (and some different version of numpy or numba didn't break the code)? You might want to add statements to the user guide that make it explicit what attributes the user should check to ensure the code is properly functioning.
To check model function, we have included the values for simulated O3 concentrations from the default version of the model in a csv format in the test files folder. Any change in the function of the packages will change the outputs of the model.We chose ozone as it has many links to chemical and physical transformations in the model and is a good indicator species. Clearly though, the user would need to run the model in the default condition to be able to reproduce the ozone concentrations. A test script already exists to test the validity of the functions that build the system of ODEs. I have added this method of testing model function into the manual.
Thank you again for taking the time to review our software and suggest useful improvements.
:wave: @goldmanm, please update us on how your review is going (this is an automated reminder).
:wave: @khinsen, please update us on how your review is going (this is an automated reminder).
@whedon generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@DrDaveShaw Thank you for implementing all these changes. I am going to double check documentation creation and rerun the example and analysis to make sure it is all functioning properly.
Towards the end of this process, it might be useful to create a new version number and mention this version number in the manuscript for the package (so users can easily know exactly what software version the manuscript corresponds to, in case additional chemistry or functionality is added later). Maybe v1.1 or v1.0.1 or v2, your choice.
I'm also considering running some other cases to double check that what I expect to occur is what occurs.
@goldmanm I believe we'll go to v1.1 after this review process so I will change the manuscript accordingly.
I am still working my way through issues raised by @khinsen on the repository itself so I don't know if you would prefer to wait until they are complete to run your checks?
Thank you again and let me know if there's anything else you would like us to address.
@DrDaveShaw @dhhagan Sorry for the delay - here comes my review! Thanks to the thorough first review by @goldmanm, the current version of code and paper are already much improved, which simplified my task.
INCHEM-Py belongs to the rather peculiar category of scientific software called "computational models". It is not a program/library that implements a model, it is the model. In theory, the model is a large set of differential equations with lots of parameters, but it's too large to be written down in any other useful form than software that computes solutions of the equations. The JOSS reviewer checklist needs some interpretation for computational models. The "functionality" it refers to is rather simple: run a simulation. There isn't much of an API to document or test. The real value of this software is the science it encodes. But the science is hard to document precisely, and even harder to test.
The provided documentation does a good job of describing what the software does, but readers will have to consult at least the reference Carslaw (2007) to understand how the model works. Unfortunately this is a paywalled article. Potential users who want to convince themselves that the software actually implements the science correctly will have to read the source code (as for any computational model). For this reason, I went through the code and opened a few issues on the repository to suggest improvements to readability. Note however that I did not check conformity to the description in the paper cited above, as that would be a huge effort. Validity checks will have to be made continuously by the user community, and for that reason I can only applaud the authors' decision to thoroughly document and publish the code.
Concerning the software paper, I have nothing to add to @goldmanm's suggestions, which are already implemented in the current version.
@khinsen thank you very much for your review, we appreciate the time taken and the level of detail given with the issues raised. I am working through these readability issues now and will have them complete for version 1.1 of the model to accompany the paper.
With regards to the workings of the model and consulting Carslaw (2007) and other referenced/paywalled works. It is our intention to write up fully the science modelled in detail, in a single open access publication, as we are aware that its development has been over a number of years and a number of papers. This will provide a greater level of accessibility and confidence in the software, hopefully growing the community of users.
Thanks again all for all of the support and comments, I believe the model is ready for a release alongside the paper as v1.1. @goldmanm how are the additional cases going? Is there anything I can help with?
I made a pull request improving issues with document generation, allowing the test case to run on my computer, and more intuitive messaging when finishing a run. https://github.com/DrDaveShaw/INCHEM-Py/pull/8
Look over the changes in the pull request and let me know if there are any issues. If there aren't, merge it to the main code.
I just checked off the last two boxes. Let me know if there's anything else I need to do to get this approved and published.
@drdaveshaw Thank you for ensuring your code can be used to further advance the important field of indoor air quality. I'm sure many people will find this useful!
I've just merged the pull request, all good changes and very happy with how the code has improved during this process! @goldmanm Thank you very much for taking the time to suggest these changes and for your kind support of the model.
@dhhagan please let me know if there is anything else you need me to do. Obviously the paper refers to version 1.1 which I am ready to release with all of the review changes incorporated. I will do this as soon as I am given the go ahead.
Thank you again for all of your help.
Just chiming in to say that for me this is ready for printing. Virtually of course ;-)
Awesome, @khinsen - sorry for the delay here. Been a busy few weeks!
@whedon generate pdf
@whedon check references
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1021/acs.est.5b02241 is OK
- 10.1021/es301350x is OK
- 10.1016/S1352-2310(96)00105-7 is OK
- 10.1016/j.atmosenv.2013.09.042 is OK
- 10.1016/j.atmosenv.2020.117784 is OK
- 10.1111/ina.12402 is OK
- 10.1111/ina.12394 is OK
- 10.1039/c9em00140a is OK
- 10.1016/j.atmosenv.2013.08.034 is OK
- 10.1111/ina.12381 is OK
- 10.1111/ina.12702 is OK
- 10.1145/2833157.2833162 is OK
- 10.1016/j.atmosenv.2006.09.038 is OK
- 10.21105/joss.00755 is OK
- 10.5194/gmd-13-169-2020 is OK
- 10.21105/joss.01918 is OK
- 10.5194/acp-3-161-2003 is OK
- 10.1016/1352-2310(94)90094-9 is OK
MISSING DOIs
- None
INVALID DOIs
- None
@DrDaveShaw when you get a chance, could you create a tagged release and post the following:
Once I have those, I will proceed to acceptance! Sorry again for the delay!
@dhhagan the final version number is v1.1 and the archive can be found at 10.5281/zenodo.5036374
Thank you very much for all of your help!
@DrDaveShaw Sorry for the long delay here. Can you update the title in Zenodo to match that of the paper? I think that's the last thing that needs to be fixed pre-publication.
@dhhagan fixed.
@whedon set 10.5281/zenodo.5036374 as archive
OK. 10.5281/zenodo.5036374 is the archive.
@whedon set v1.1 as version
OK. v1.1 is the version.
@whedon recommend-accept
Attempting dry run of processing paper acceptance...
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1021/acs.est.5b02241 is OK
- 10.1021/es301350x is OK
- 10.1016/S1352-2310(96)00105-7 is OK
- 10.1016/j.atmosenv.2013.09.042 is OK
- 10.1016/j.atmosenv.2020.117784 is OK
- 10.1111/ina.12402 is OK
- 10.1111/ina.12394 is OK
- 10.1039/c9em00140a is OK
- 10.1016/j.atmosenv.2013.08.034 is OK
- 10.1111/ina.12381 is OK
- 10.1111/ina.12702 is OK
- 10.1145/2833157.2833162 is OK
- 10.1016/j.atmosenv.2006.09.038 is OK
- 10.21105/joss.00755 is OK
- 10.5194/gmd-13-169-2020 is OK
- 10.21105/joss.01918 is OK
- 10.5194/acp-3-161-2003 is OK
- 10.1016/1352-2310(94)90094-9 is OK
MISSING DOIs
- None
INVALID DOIs
- None
:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.
Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/2433
If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/2433, then you can now move forward with accepting the submission by compiling again with the flag deposit=true
e.g.
@whedon accept deposit=true
@whedon accept deposit=true
I'm sorry @DrDaveShaw, I'm afraid I can't do that. That's something only editor-in-chiefs are allowed to do.
π nice try
@whedon check references
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1021/acs.est.5b02241 is OK
- 10.1021/es301350x is OK
- 10.1016/S1352-2310(96)00105-7 is OK
- 10.1016/j.atmosenv.2013.09.042 is OK
- 10.1016/j.atmosenv.2020.117784 is OK
- 10.1111/ina.12402 is OK
- 10.1111/ina.12394 is OK
- 10.1039/c9em00140a is OK
- 10.1016/j.atmosenv.2013.08.034 is OK
- 10.1111/ina.12381 is OK
- 10.1111/ina.12702 is OK
- 10.1145/2833157.2833162 is OK
- 10.1016/j.atmosenv.2006.09.038 is OK
- 10.21105/joss.00755 is OK
- 10.5194/gmd-13-169-2020 is OK
- 10.21105/joss.01918 is OK
- 10.5194/acp-3-161-2003 is OK
- 10.1016/1352-2310(94)90094-9 is OK
MISSING DOIs
- None
INVALID DOIs
- None
Ok everything looks good!
@whedon accept deposit=true
Doing it live! Attempting automated processing of paper acceptance...
π¦π¦π¦ π Tweet for this paper π π¦π¦π¦
π¨π¨π¨ THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! π¨π¨π¨
Here's what you must now do:
Party like you just published a paper! πππ¦ππ»π€
Any issues? Notify your editorial technical team...
Congrats on your new publication @DrDaveShaw! And many thanks to editor @dhhagan and reviewers @goldmanm and @khinsen for your time, hard work, and expertise!
(leaving this issue open until the DOI resolves)
Submitting author: @DrDaveShaw (David Shaw) Repository: https://github.com/DrDaveShaw/INCHEM-Py Version: v1.1 Editor: @dhhagan Reviewer: @goldmanm, @khinsen Archive: 10.5281/zenodo.5036374
: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
@goldmanm & @khinsen, 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 @goldmanm
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @khinsen
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper