Skip to content
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

JOSS paper #107

Closed
16 tasks done
joethorley opened this issue Dec 5, 2024 · 1 comment
Closed
16 tasks done

JOSS paper #107

joethorley opened this issue Dec 5, 2024 · 1 comment

Comments

@joethorley
Copy link
Member

joethorley commented Dec 5, 2024

Thanks

Thanks for everyone comments and suggestions. Below are your comments with our response. I added the tick box to indicate that we had responded to the comment. Hopefully our changes and responses are adequate.

First Reviewer

  • Issue: In almost all the vignettes there seems to be a bug in the formulas that are inside a grey box.

Fixed by closing bcgov#387.

  • There are also issues with the rendering of images and formulas in the "Additional Technical Details" vignette.

Fixed (can't find the exact issue or commit - sorry)

  • The NEWS section mentions that some of the changes introduced have effects ("The following changes are major in the sense that they could alter previous hazard concentrations or break code".), either by adding arguments to the functions and/or altering the potential results of functions compared to the original version.
    It could be beneficial to briefly mention in the internal documentation of these specific functions that significant changes were made in version X of the package. This would help ensure that users of previous versions are aware of these modifications and prevent errors of interpretation when comparing new outputs with previous results or results of other articles using the same software.
    I know this information is also in the changelog, but not all the users will check it. This package was created for researchers who do not necessarily have a background in software development. It is more likely that users will consult the function documentation.

We have gone through the documentation and ensured that all functions or arguments that are deprecated have a lifecycle badge indicating this and where an alternative function/argument is available this is indicated both in the documentation and when the function is called. We have not attempted to document changes in the functions themselves because its complicated and often partly dependent on other functions and we are concerned it might cause confusion and that the function documentation should indicate what the functions currently do. We are of the opinion that it is the purpose of this JOSS paper to give a higher level overview and the NEWS file to provide information on when specific changes occurred and what they were.

  • Yes, the description is clear and easy to understand. The term "toxicological data" may not be very informative for a non-specialist audience. It might be interesting to include some minimal requirements about the type of data used to construct these curves, as the data selection can change the results. The documentation mentions ssdata as a source of datasets, but it is not the only possibility.

The DESCRIPTION says 'toxicity concentrations' as opposed to 'toxicological data' and provides a reference and as its supposed to be succinct I think its ok as is. The minimum data requirements (n) change by jurisdiction and there is an argument the user can set so in this sense the package does not really set a minimal data requirement. I have added a reference to ECOTOX as an alternative source of data. bcgov@2aed846

  • This package has a very specific use, so there aren't many others to compare it with. However, in the context of this review, the change in an important dependency as fitdistrplus to TMB might deserve a bit more attention. The article mentions that this change promotes greater control over model specifications. How exactly does this change affect the user experience with the functions in ssdtools? Could a line be added to explain the existence of new functions or what specific changes to expect?

The paper now states that "The move to TMB means the likelihood function is hand coded in C++, which allows full control over model specification and improved handling of censored data. The change is internal and does not directly affect the user interface."
bcgov@2ea7641

  • This comment is similar to the previous one but a bit more general regarding the Technical Details section. In some subsections, such as Multiple Distribution Functions and Plotting, the technical changes introduced are accompanied by the ssdtools functions they affect between brackets. On the other hand, other sections, like Model Averaging and Model Fitting, are more theoretical.
    The inclusion of formulas is important in a statistically oriented package like ssdtools. However, since this publication emphasizes software development, I would suggest adding a code example. This could minimally involve mentioning the functions affected by these changes (as done in other subsections) or including a few lines of code to demonstrate how the explained concepts translate into changes in the package's functionality.
    The ssdtools vignettes are very detailed and combine well code and the statistical theory, they could also be cited to avoid repeating information. My comment is more about linking the code with the theory/changes explicitly.

We've edited the text in the paper to include more code bcgov@89e9a34
We are reluctant to cite the vignettes in case the change overtime breaking the links in the paper.

  • This update includes two significant changes in the versions of ssdtools. It might be helpful to organize the Technical Details section to indicate which changes were introduced in the first major version update and which were added in the second one.

Considered closed as editor indicated that this level of historical detail seems less relevant to them, as the new paper is supposed to document the current state of the software.

Second Reviewer

Fixed (can't find the exact issue or commit - sorry!)

  • Given the availability of packages like 'viridis' designed for color-blind accessibility, any specific purpose of scale_color_ssd()? Additionally, since it only offers eight colors, what would you recommend for datasets with more than eight groups?

It was developed to color different groups which are unlikely to exceed 8. Provide more description in bcgov@94effc5

  • There are currently no examples illustrating scale_fill_ssd() in SSD analysis. What is the application of this function?

Added example bcgov@94effc5

  • The description only states "Censor Data". I suggest adding further explanation of its purpose and how it fits into typical workflows?

The description now states that 'Censors data to a specified range based on the censoring argument. The function is useful for creating test data sets.'

  • I guess multi_est is related to ci_method. The method for integrating multi_est and ci_method is unclear. Additional guidance on their use would be helpful.

We've edited the text in the paper to clarify. bcgov@89e9a34

  • In the Model Fitting section, "The move to TMB allowed more control over model specification" is vague. More details are needed to explain what option that can be used in model specification.

The paper now states that "The move to TMB means the likelihood function is hand coded in C++, which allows full control over model specification and improved handling of censored data. The change is internal and does not directly affect the user interface."
bcgov@2ea7641

  • In the section of Multiple Distribution Function. It might need more explanation about how these functions implement in the "multi" method.

We've moved the text into the section on confidence intervals to clarify. bcgov@89e9a34

  • The function ssd_xxmulti_fitdists() is referenced in the ChangeLog but does not appear in the package.

Updated NEWS file to indicate that xx refers to a family of functions bcgov@5e294b3

  • Some warnings were found when using pkgdown building package site [This is the alt-text].

Fixed by closing bcgov#182.

Moving forward

  • We have added @flor14 as a contributor to ssdtools. @nanhung was already a contributor based on his review of our previous paper.

We have bumped the software to v2.2.0 and as soon as it is accepted by JOSS we will submit to CRAN.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant