Closed editorialbot closed 5 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.05 s (960.8 files/s, 254527.0 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
R 36 684 3839 3857
JSON 1 0 0 2597
Markdown 2 126 0 491
YAML 5 41 14 196
TeX 1 6 0 70
-------------------------------------------------------------------------------
SUM: 45 857 3853 7211
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repository
Wordcount for paper.md
is 796
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.2136/sssaj1981.03615995004500020017x is OK
- 10.1111/j.1365-2389.2010.01291.x is OK
- 10.1371/journal.pone.0200876 is OK
- 10.21105/joss.01751 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:
@Schuch666, @shubhamjain15, @hahsan1 – This is the review thread for the paper. All of our communications will happen here from now on.
Please read the "Reviewer instructions & questions" in the first comment above. Please create your checklist typing:
@editorialbot generate my checklist
As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.
The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention https://github.com/openjournals/joss-reviews/issues/6393
so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.
We aim for the review process to be completed within about 4-6 weeks but please make a start well ahead of this as JOSS reviews are by their nature iterative and any early feedback you may be able to provide to the author will be very helpful in meeting this schedule.
Hi @Qepanna, You mention in the installation section that "must first install the package devtools", but the user can use another package that provide the same functionality such as the remotes
package:
remotes::install_github('qepanna/goflux')
can you include a note that include remotes?
Hi @Qepanna, Could you take a look at this error in the 'uCH4_import' and 'uN2O_import' functions. My guess is you need to remove the highlighted lines (attached image) in the code which may have been for testing purposes only.
library(goFlux)
file.path <- system.file("extdata", "uCH4/uCH4.txt", package = "goFlux")
uCH4_imp <- uCH4_import(inputfile = file.path)
file.path <- system.file("extdata", "uN2O/uN2O.txt", package = "goFlux")
uN2O_imp <- uN2O_import(inputfile = file.path)
Warning: cannot open file 'inst/extdata/uCH4/uCH4.txt': No such file or directoryWarning: Error occurred in file uCH4.txt: Error in file(file, "rt"): cannot open the connection Warning: cannot open file 'inst/extdata/uN2O/uN2O.txt': No such file or directoryWarning: Error occurred in file uN2O.txt: Error in file(file, "rt"): cannot open the connection
@Qepanna In the 'goFlux' function, the input dataframe allows for chamID to be used instead of UniqueID, but when I go to plot the results using flux.plot, it throws an error:
library(goFlux)
CO2_flux <- goFlux(LI8100_imp, "CO2dry_ppm")
CO2_flux_res <- best.flux(CO2_flux)
CO2_flux_plots <- flux.plot(CO2_flux_res, LI8100_imp, "CO2dry_ppm", shoulder=20,
plot.legend, plot.display, quality.check)
Error in flux.plot(CO2_flux_res, LI8100_imp, "CO2dry_ppm", shoulder = 20, : 'UniqueID' is required and was not found in 'dataframe'
The argument description for the flux.plot function says the same dataframe can be used for both goFlux and flux.plot. Would it be possible to allow chamID in flux.plot as well?
Hi @Qepanna, You mention in the installation section that "must first install the package devtools", but the user can use another package that provide the same functionality such as the
remotes
package:
remotes::install_github('qepanna/goflux')
can you include a note that include remotes?
Hi @Schuch666, originally it was included that one could use the remotes packages, however I encounterd some errors using this package and I thought it would be best to avoid using it. Would you still recommend that I include the package remotes?
Hi @Qepanna, Could you take a look at this error in the 'uCH4_import' and 'uN2O_import' functions. My guess is you need to remove the highlighted lines (attached image) in the code which may have been for testing purposes only.
library(goFlux) file.path <- system.file("extdata", "uCH4/uCH4.txt", package = "goFlux") uCH4_imp <- uCH4_import(inputfile = file.path) file.path <- system.file("extdata", "uN2O/uN2O.txt", package = "goFlux") uN2O_imp <- uN2O_import(inputfile = file.path)
Warning: cannot open file 'inst/extdata/uCH4/uCH4.txt': No such file or directoryWarning: Error occurred in file uCH4.txt: Error in file(file, "rt"): cannot open the connection Warning: cannot open file 'inst/extdata/uN2O/uN2O.txt': No such file or directoryWarning: Error occurred in file uN2O.txt: Error in file(file, "rt"): cannot open the connection
Hi @shubhamjain15, thank you for pointing that out. I fixed these functions and they now seem to work.
@Qepanna In the 'goFlux' function, the input dataframe allows for chamID to be used instead of UniqueID, but when I go to plot the results using flux.plot, it throws an error:
library(goFlux) CO2_flux <- goFlux(LI8100_imp, "CO2dry_ppm") CO2_flux_res <- best.flux(CO2_flux) CO2_flux_plots <- flux.plot(CO2_flux_res, LI8100_imp, "CO2dry_ppm", shoulder=20, plot.legend, plot.display, quality.check)
Error in flux.plot(CO2_flux_res, LI8100_imp, "CO2dry_ppm", shoulder = 20, : 'UniqueID' is required and was not found in 'dataframe'
The argument description for the flux.plot function says the same dataframe can be used for both goFlux and flux.plot. Would it be possible to allow chamID in flux.plot as well?
Hi @shubhamjain15, this is a very good point. I added the option to the flux.plot function to use chamID instead of UniqueID.
Thanks @Qepanna, it works for me. Another quick question - While loading the "DX4015.TXT" file, it only loads the first 14 rows upto time - 08:20:35. However, in the txt file, I do see more rows that go upto 08:30:44. Is there any specific reason why those rows were not read or excluded by the import function?
library(goFlux)
file.path <- system.file("extdata", "DX4015/DX4015.TXT", package = "goFlux")
DX4015_imp <- DX4015_import(inputfile = file.path)
print(nrow(DX4015_imp))
[1] 14
Hi @Qepanna, You mention in the installation section that "must first install the package devtools", but the user can use another package that provide the same functionality such as the
remotes
package:remotes::install_github('qepanna/goflux')
can you include a note that include remotes?Hi @Schuch666, originally it was included that one could use the remotes packages, however I encounterd some errors using this package and I thought it would be best to avoid using it. Would you still recommend that I include the package remotes?
Hello @Qepanna,
I did not have any issues installing using remotes (for me I use remotes for R installations that don't need all the developing tools from devtools because remotes is lighter), but it is up to you to include or not instructions to use remotes (If users still need to avoid remotes to install your package). But instead of "must first install devtools" in the installation instructions should be something less restrictive.
PS: is possible that the issue with your package / remotes is already fixed because both packages may have several updates
Dear all, there is my general comments:
In general the documentation is very good, and the documentation (specially the website) has god additional technical content.
I found some small errors running some of the "import raw data into R" examples but the last version of the package was this errors already fixed.
And a few questions/suggestions:
click.peak
function? This plot can be useful (https://github.com/Qepanna/goFlux/issues/2#issue-2176867056)data(LGR_manID)
that need to be included in the example.goFlux
functiuon: if the function is called without an argument gastype
, can be included a menu to choose the variable or a print message with the variable names (or maybe a print of the variables that can be used and an error if this is part of the desing). (https://github.com/Qepanna/goFlux/issues/1#issue-2176863327)data(LGR_manID)
that need to be included in the example.CO2_flux_res
and CH4_flux_res
is missing, this can be found in previous page but should be better include all stepsI have included 3 issues with my suggestions on the issue page
There is an inconsistency on the naming convention on the functions: You have function with the name best.flux
(with a dot) flux2pdf
(a number), goFlux
(a capital letter), EGM5_import
(using underline) and goFlux_nokmax
(using an underline and a capital letter). I suggest that you rename these files to match with only one naming standard.
For the functions for specific instruments: some cases the names are not very informative to the user for LI7820_imp (this is a dataset from LI7820 instrument), and for other cases change the order LI7820_import to import_LI7820 make more sense in the organization of the documentation and help to write the code (but this is up to you to change or not).
I have successfully installed the package and tested out various functions in the documentation. The documentation is very thorough and detailed. It may be helpful to have a separate bare bones "Example" section where it runs through each step (e.g., import, goFlux, best.flux, plotting) for just a single data set. The documentation currently has a fair bit of methodology/equations that could potentially be an Appendix.
Thanks @Qepanna, it works for me. Another quick question - While loading the "DX4015.TXT" file, it only loads the first 14 rows upto time - 08:20:35. However, in the txt file, I do see more rows that go upto 08:30:44. Is there any specific reason why those rows were not read or excluded by the import function?
library(goFlux) file.path <- system.file("extdata", "DX4015/DX4015.TXT", package = "goFlux") DX4015_imp <- DX4015_import(inputfile = file.path) print(nrow(DX4015_imp))
[1] 14
Hi @shubhamjain15, thank you for pointing that out. It was part of all import functions that negative values should be removed. However, there was a mistake and 0 values were also removed. I now modified all import function to keep 0 values.
Hi @Qepanna, You mention in the installation section that "must first install the package devtools", but the user can use another package that provide the same functionality such as the
remotes
package:remotes::install_github('qepanna/goflux')
can you include a note that include remotes?Hi @Schuch666, originally it was included that one could use the remotes packages, however I encounterd some errors using this package and I thought it would be best to avoid using it. Would you still recommend that I include the package remotes?
Hello @Qepanna,
I did not have any issues installing using remotes (for me I use remotes for R installations that don't need all the developing tools from devtools because remotes is lighter), but it is up to you to include or not instructions to use remotes (If users still need to avoid remotes to install your package). But instead of "must first install devtools" in the installation instructions should be something less restrictive.
PS: is possible that the issue with your package / remotes is already fixed because both packages may have several updates
I included that both packages can be used and I just found out that the issue can arise with either packages. The issue concerns the API rate limit. There is a troubleshoot section about that in the README file and the webpage. However, it anoys me that users are required to create a GitHub account to avoid that problem. Do you know if there is any way to avoid this issue altogether?
Dear all, there is my general comments:
In general the documentation is very good, and the documentation (specially the website) has god additional technical content.
I found some small errors running some of the "import raw data into R" examples but the last version of the package was this errors already fixed.
And a few questions/suggestions:
- In the paper/documentation there is "identify measurements", can you include more information (identification of the instrument? identification of start/end of measures? variables that are measured? other flags?)
- For the visualization part (https://qepanna.quarto.pub/goflux/flux2pdf.html), there is a part that change the timezone of R environment, I recommend to include as a tip instead of the example.
- Can an option to save the window or disable the auto-close of the
click.peak
function? This plot can be useful (Suggestion: keep the plots from click.peak Qepanna/goFlux#2 (comment))- In the goFlux webpage (https://qepanna.quarto.pub/goflux/goFlux.html), there is a
data(LGR_manID)
that need to be included in the example.- I have a recommendation to the
goFlux
functiuon: if the function is called without an argumentgastype
, can be included a menu to choose the variable or a print message with the variable names (or maybe a print of the variables that can be used and an error if this is part of the desing). (sugestion: goFlux without gastype Qepanna/goFlux#1 (comment))- What is the difference of goFlux and best.flux? Is the first one always combines LM and HM and the second one use some criteria and calculate the flux using LM or HM depending of the results? There is any diagnostic output to inform the user more about the best method or how the output was calculated (some variables on the output, a message or some kind of log file)? (Diagnostic output from goFlux function Qepanna/goFlux#3 (comment))
- In the website (https://qepanna.quarto.pub/goflux/bestflux.html), there is a
data(LGR_manID)
that need to be included in the example.- In the website (https://qepanna.quarto.pub/goflux/flux2pdf.html), part of the example is on different box (tip) and the how to calculate
CO2_flux_res
andCH4_flux_res
is missing, this can be found in previous page but should be better include all steps- On the website (https://qepanna.quarto.pub/goflux/import.html#mgga-gla131-series) the file used on the examples from UGGA and MGGA are the same, are they suppose to be the same (with identical format)?
I have included 3 issues with my suggestions on the issue page
Dear @Schuch666, thank you for all your suggestions. I have modified the related functions and the webpage accordingly for the most part. Here are my detailed answers to your comments. I also copied those answers on the corresponding issues you oppened.
Error in goFlux(LGR_manID, gastype = NULL) :
'gastype' must be one of the following: 'CO2dry_ppm', 'CH4dry_ppb', 'COdry_ppb', 'N2Odry_ppb', 'NH3dry_ppb' or 'H2O_ppm'
Thanks @Qepanna. Great work! @arfon I have completed my checklist and do not have any further comments or issues with this submission. Thanks for inviting me for this review.
Looks like we're close to wrapping up here. Thanks for the timely reviews folks ⚡
@Schuch666 – I can see that there are a few items left unchecked in your review checklist – are you still awaiting changes from the author on these matters or do you believe your feedback has been incorporated and these can now be checked off?
Dear all,
@Qepanna
For 2-9 everything is fine.
About the issue with remotes and API rate limit, I'm not sure, can you create a issue with the steps to reproduce the error and tag me?
I don't have a reply about the inconsistency / non-explanatory names used on the package and I'm including this as a suggestion on the issue session of the package repository.
Thank you
@arfon for my part the only remaining issue is my comment 1
There is an inconsistency on the naming convention on the functions: You have function with the name
best.flux
(with a dot)flux2pdf
(a number),goFlux
(a capital letter),EGM5_import
(using underline) andgoFlux_nokmax
(using an underline and a capital letter). I suggest that you rename these files to match with only one naming standard.For the functions for specific instruments: some cases the names are not very informative to the user for LI7820_imp (this is a dataset from LI7820 instrument), and for other cases change the order LI7820_import to import_LI7820 make more sense in the organization of the documentation and help to write the code (but this is up to you to change or not).
Dear @Schuch666, I answered this comment in the issue you opened.
I have successfully installed the package and tested out various functions in the documentation. The documentation is very thorough and detailed. It may be helpful to have a separate bare bones "Example" section where it runs through each step (e.g., import, goFlux, best.flux, plotting) for just a single data set. The documentation currently has a fair bit of methodology/equations that could potentially be an Appendix.
Dear @hahsan1, I agree that an overall example is necessary. I had planned to make downloadable scripts for each pathway. Could you open an issue with your suggestion?
- define the start and end points of each measurement and assign a UniqueID
Dear @Schuch666, I corrected the paper with your suggestion.
Dear all,
@Qepanna
- This comment is specific to the paper summary "2. identify measurements", replace for "define the start and end points of each measurement and assign a UniqueID", identify measurements is not self explanatory.
For 2-9 everything is fine.
About the issue with remotes and API rate limit, I'm not sure, can you create a issue with the steps to reproduce the error and tag me?
I don't have a reply about the inconsistency / non-explanatory names used on the package and I'm including this as a suggestion on the issue session of the package repository.
Thank you
Dear @Schuch666, I created an issue regarding the API rate limit and answered the other two comments.
Looks like we're close to wrapping up here. Thanks for the timely reviews folks ⚡
@Schuch666 – I can see that there are a few items left unchecked in your review checklist – are you still awaiting changes from the author on these matters or do you believe your feedback has been incorporated and these can now be checked off?
Dear, @arfon, I am only missing two comments to answer:
I have posted an issue highlighting the suggestion to add a standalone example in the documentation. I have no further comments to add. Great work on the package!
@arfon @Schuch666 @shubhamjain15 @hahsan1 Hi everyone, thank you for the fast review process and the great suggestions! I answered all comments and believe that the package is ready.
OK, thanks for the great discussion here and the iterative improvements throughout the review!
@Qepanna – looks like we're very close to being done here. I will circle back here next week, but in the meantime, please give your own paper a final read to check for any potential typos etc.
After that, could you make a new release of this software that includes the changes that have resulted from this review. Then, please make an archive of the software in Zenodo/figshare/other service and update this thread with the DOI of the archive? For the Zenodo/figshare archive, please make sure that:
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
I made some modifications and I found that the mention "* These authors contributed equally" should be removed.
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
OK, thanks for the great discussion here and the iterative improvements throughout the review!
@Qepanna – looks like we're very close to being done here. I will circle back here next week, but in the meantime, please give your own paper a final read to check for any potential typos etc.
After that, could you make a new release of this software that includes the changes that have resulted from this review. Then, please make an archive of the software in Zenodo/figshare/other service and update this thread with the DOI of the archive? For the Zenodo/figshare archive, please make sure that:
- The title of the archive is the same as the JOSS paper title
- That the authors of the archive are the same as the JOSS paper authors
- I can then move forward with accepting the submission.
Hi @arfon, here is the DOI from Zenodo: https://zenodo.org/doi/10.5281/zenodo.10908262
@editorialbot set 10.5281/zenodo.10908262 as archive
Done! archive is now 10.5281/zenodo.10908262
@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
Attempting dry run of processing paper acceptance...
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.2136/sssaj1981.03615995004500020017x is OK
- 10.1111/j.1365-2389.2010.01291.x is OK
- 10.1371/journal.pone.0200876 is OK
- 10.21105/joss.01751 is OK
MISSING DOIs
- No DOI given, and none found for title: flux: Flux Rate Calculation from Dynamic Closed Ch...
- No DOI given, and none found for title: gasfluxes: Greenhouse Gas Flux Calculation from Ch...
INVALID DOIs
- None
:wave: @openjournals/ese-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/5207, then you can now move forward with accepting the submission by compiling again with the command @editorialbot accept
@arfon - Looks all good to me!
Submitting author: !--author-handle-->@qepanna<!--end-author-handle-- (Karelle Rheault) Repository: https://github.com/Qepanna/goFlux Branch with paper.md (empty if default branch): Version: v0.1.0 Editor: !--editor-->@arfon<!--end-editor-- Reviewers: @Schuch666, @shubhamjain15, @hahsan1 Archive: 10.5281/zenodo.10908262
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
@Schuch666 & @shubhamjain15 & @hahsan1, 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 @arfon 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 @Schuch666
📝 Checklist for @shubhamjain15
📝 Checklist for @hahsan1