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

nlrx #262

Closed
nldoc opened this issue Oct 19, 2018 · 45 comments

Comments

@nldoc
Copy link

commented Oct 19, 2018

Summary

  • What does this package do? (explain in 50 words or less):

nlrx enables you to run agent-based models in NetLogo from R.
This makes it possible to design reproducible experiments (which can be
zipped with nlrx and shared between persons). Furthermore, it
coerces the spatial data from NetLogo into raster and sf objects.

  • Paste the full DESCRIPTION file inside a code block below:
Package: nlrx
Type: Package
Title: Setup, run and analyze NetLogo model simulations from R via XML
Version: 0.1.0
Authors@R: c(person("Jan", "Salecker", email = "jsaleck@gwdg.de",
                     role = c("aut", "cre"), comment = c(ORCID = '0000-0002-9000-4229')),
             person("Marco", "Sciaini", email = "sciaini.marco@gmail.com", role = c("aut"), comment = c(ORCID = '0000-0002-3042-5435')))
Maintainer: Jan Salecker <jsaleck@gwdg.de>
Description: The nlrx package provides tools to setup, run and analyze NetLogo model simulations in R.
    nlrx experiments use a similar structure as NetLogos Behavior Space experiments.
    However, nlrx offers more flexibility and additional tools for running and anlyzing complex simulation designs and sensitivity analyses.
    The user defines all information that is needed in an intuitive framework, using S4 class objects.
    Experiments are submitted from R to NetLogo via XML files that are dynamically written, based on specifications defined by the user.
    By nesting model calls in future environments, large simulation design with many runs can be executed in parallel.
    This also enables simulating NetLogo experiments on remote HPC machines.
License: GPL-3
Encoding: UTF-8
LazyData: true
Roxygen: list(markdown = TRUE)
RoxygenNote: 6.1.0
URL: https://github.com/nldoc/nlrx/
BugReports: https://github.com/nldoc/nlrx/issues/
Imports:
    XML,
    lhs,
    sensitivity,
    dplyr,
    readr,
    purrr,
    furrr,
    magrittr,
    stats,
    utils,
    GenSA,
    genalg,
    raster,
    rstudioapi,
    sf,
    tidyr
Depends:
   methods,
    tibble,
    R (>= 2.10)
Suggests:
    testthat,
    knitr,
    rmarkdown
VignetteBuilder: knitr
  • URL for the package (the development repository, not a stylized html page):

  • https://github.com/nldoc/nlrx

  • Please indicate which category or categories from our package fit policies this package falls under *and why(? (e.g., data retrieval, reproducibility. If you are unsure, we suggest you make a pre-submission inquiry.):

    • reproducibility, as it is the first package that lets you perform systematic packages with NetLogo from R
    • partly spatial, as the landscapes from NetLogo are retrieved as rasters and the agents as sf objects
  •   Who is the target audience and what are scientific applications of this package?  

    • The audience is the scientific NetLogo community, that performs experiments within
      NetLogo. They now can use nlrx to parameterise NetLogo and scale their experiments up.
      With nlrx it is the first also possible to run NetLogo experiments on computing clusters.
  • Are there other R packages that accomplish the same thing? If so, how does
    yours differ or meet our criteria for best-in-category?

  • There is RNetLogo on CRAN, this package however is based on rJava (which is most often an obstacle) and has not as much auxiliary functions to run NetLogo and embed results in R objects.

Requirements

Confirm each of the following by checking the box. This package:

  • does not violate the Terms of Service of any service it interacts with.
  • has a CRAN and OSI accepted license.
  • contains a README with instructions for installing the development version.
  • includes documentation with examples for all functions.
  • contains a vignette with examples of its essential functions and uses.
  • has a test suite.
  • has continuous integration, including reporting of test coverage, using services such as Travis CI, Coveralls and/or CodeCov.
  • I agree to abide by ROpenSci's Code of Conduct during the review process and in maintaining my package should it be accepted.

Publication options

  • Do you intend for this package to go on CRAN?
  • Do you wish to automatically submit to the Journal of Open Source Software? If so:
    • The package has an obvious research application according to JOSS's definition.
    • The package contains a paper.md matching JOSS's requirements with a high-level description in the package root or in inst/.
    • The package is deposited in a long-term repository with the DOI:
    • (Do not submit your package separately to JOSS)
  • Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:
    • The package is novel and will be of interest to the broad readership of the journal.
    • The manuscript describing the package is no longer than 3000 words.
    • You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see MEE's Policy on Publishing Code)

Detail

  • Does R CMD check (or devtools::check()) succeed? Paste and describe any errors or warnings:

  • Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:

  • If possible, please provide recommendations of reviewers - those with experience with similar packages and/or likely users of your package - and their GitHub user names:

    • Robin Lovelace (@Robinlovelace i), as he has experience with spatial simulations and NetLogo
@sckott

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

thanks for your submission @nldoc we're discussing and will get back to you soon

@sckott

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

@nldoc I completely agree the lack of rJava dependency is a big plus. . But can you comment more on the benefit of your pkg over RNetLogo with respect to features/functionality.

@sckott

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

There's also https://cran.rstudio.com/web/packages/NetLogoR/ - not sure if you were aware or not? Can you also tell us benefit of your pkg over NetLogoR as well please

@nldoc

This comment has been minimized.

Copy link
Author

commented Oct 20, 2018

@nldoc I completely agree the lack of rJava dependency is a big plus. . But can you comment more on the benefit of your pkg over RNetLogo with respect to features/functionality.

The main purpose of nlrx is running a NetLogo model and collecting output data. Everything is designed to make this as simple and reproducible as possible.

Reproducability

  • nlrx uses nested S4 classes to store all the information that is needed to run NetLogo model simulations.
  • After running simulations, the model output is attached to the S4 class object, thus the complete experiment including all experiment definitions, the parameter input matrix, runtime, etc. is now stored in one single S4 class object that can be easily archived and shared with other users.
  • For complete reproducability the NetLogo model files are needed. Thus, nlrx provides a function to generate a zip file from the model folder, including a rds file containing the S4 class object that holds the information on the experiment. This zip file could be given to anyone to reproduce nlrx NetLogo simulations.
  • When using RNetLogo, everything has to be written from scratch and there is no easy way to share and reproduce complex simulation experiments.

Setting up experiments

  • Because everything in nlrx is designed on running NetLogo simulations (and in particular Sensitivity Analysis), the setup is very intuitive and requires a minimum amount of code to be written.
  • RNetLogo may be more general in its usage than nlrx because it allows controlling interactive NetLogo sessions from R, however to set up large sensitivity analysis experiments a lot of R code needs to be written.
  • The nlrx design is in many parts adapted from the NetLogo built-in tool behavior space, which allows easy understanding of experiment setup for users that are already familiar with NetLogo

Auxiliary functions

  • The nlrx package provides a collection of simulation design helper functions. These can be used to create parameter input matrices for different simulation methods, based on the defined model variables.
  • For example, the simdesign_ff() creates a full factorial parametrer matrix. simdesign_morris() creates a morris screening parameter matrix (there are many more).
  • After running a sensitivity analysis simdesign created with one of these helper functions (e.g. simdesign_morris()), nlrx provides functions to calculate sensitivity indices for each parameter and output
  • RNetLogo does not provide any auxiliary functions to setup simulations, generate input parameter matrices or analyze model output
  • Additionally, nlrx provides auxiliary functions that make it easy to convert spatial output from NetLogo models to spatial R formats (patches are stored as raster objects and turtles are stored as sf point objects). RNetLogo only provides functions to collect spatial data from NetLogo as data frames.

Summary
The main purpose of the nlrx package was not to replace RNetLogo. Our motivation was to develop a package that makes running sensitivity analysis with R and NetLogo easier and reproducible. While RNetLogo may be more general in its usage, nlrx has many benefits when it comes to its main task: running a NetLogo model many times, with different parameter combinations and collecting model output. For this task nlrx is better suited as RNetLogo because of easier setup (with less code), enhanced reproducability and easy exchange with other users, built-in auxiliary functions to create parameter matrices and calculate sensitivity indices, and increased stability by removing rJava dependency.

@nldoc

This comment has been minimized.

Copy link
Author

commented Oct 20, 2018

There's also https://cran.rstudio.com/web/packages/NetLogoR/ - not sure if you were aware or not? Can you also tell us benefit of your pkg over NetLogoR as well please

The NetLogoR package follows a different approach because it re-implements the NetLogo syntax in R. While this package can be used to rebuild NetLogo models in R, it can not be used to control already existing NetLogo models, which is the main purpose of the nlrx package.

@marcosci

This comment has been minimized.

Copy link

commented Oct 22, 2018

@nldoc I completely agree the lack of rJava dependency is a big plus... But can you comment more on the benefit of your pkg over RNetLogo with respect to features/functionality?

What Jan (@nldoc ) did not mention is the way the Java virtual machine is handled - nlrx opens a new one for each NetLogo model run, whereas RNetLogo keeps a single one open until you restart the R session. This can be quite cumbersome, in terms of memory, and should be solved with the way nlrx handles this.

@sckott

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

thanks for the additional information @nldoc and @marcosci - we're discussing and will get back asap

@annakrystalli

This comment has been minimized.

Copy link
Collaborator

commented Oct 25, 2018

Editor checks:

  • Fit: The package meets criteria for fit and overlap
  • Automated tests: Package has a testing suite and is tested via Travis-CI or another CI service.
  • License: The package has a CRAN or OSI accepted license
  • Repository: The repository link resolves correctly
  • Archive (JOSS only, may be post-review): The repository DOI resolves correctly
  • Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

Editor comments

Hello @nldoc 👋,

Thanks for your package submission! I look forward to learning more about it during the review. I've completed the editors checks and there's a few issues to clear before we proceed.

All tests and checks pass. However:

test coverage

17% test coverage is generally too low to be accepted currently and will likely need more tests. Particular attention is required on the scripts with 0% coverage. Is there are reason these scripts aren't currently being tested?

covr::package_coverage(pkg_dir)
nlrx Coverage: 17.55%
R/class_init.R: 0.00%
R/download_netlogo.R: 0.00%
R/export_nl.R: 0.00%
R/get_nl_spatial.R: 0.00%
R/import_nl.R: 0.00%
R/run_nl.R: 0.00%
R/simdesign_helper.R: 0.00%
R/util_eval.R: 0.00%
R/util_runnl_dyn.R: 0.00%
R/util_runnl.R: 0.00%
R/util_stuff.R: 0.00%
R/zzz.R: 0.00%
R/class_setget.R: 33.33%
R/analyze_nl.R: 94.02%
R/class_constr.R: 100.00%

Good Practice
goodpractice::gp(pkg_dir)

It is good practice to:

  • not use "Depends" in DESCRIPTION, as it can
    cause name clashes, and poor interaction with other
    packages. Use "Imports" instead.

  • avoid long code lines, it is bad for
    readability. Also, many people prefer editor windows that
    are about 80 characters wide. Try make your lines shorter
    than 80 characters. Have a look at styler pkg.

    R/class_init.R:37:1
    R/class_init.R:38:1
    R/class_init.R:39:1
    R/class_init.R:40:1
    R/class_init.R:41:1
    ... and 98 more lines
    
  • avoid calling setwd(), it changes the global
    environment. If you need it, consider using on.exit() to
    restore the working directory.

    R/export_nl.R:39:3
    R/export_nl.R:40:11
    R/export_nl.R:48:3
    
  • not import packages as a whole, as this can
    cause name clashes between the imported packages. Instead,
    import only the specific functions you need.

  • fix this R CMD check WARNING: LaTeX errors when
    creating PDF version. This typically indicates Rd
    problems.

    LaTeX errors found: ! Paragraph ended before
    \Rd@code was complete. <to be read again> \par l.100 !
    Paragraph ended before \Rd@code was complete. <to be read
    again> \par l.125
    
  • avoid 'T' and 'F', as they are just variables
    which are set to the logicals TRUE and FALSE by
    default, but are not reserved words and hence can be
    overwritten by the user. Hence, one should always use
    TRUE and FALSE for the logicals.

    R/analyze_nl.R:NA:NA
    R/analyze_nl.R:NA:NA
    
spell check

I've pulled out a few typos that looked real. Might be worth running spell check to satisfy yourself too.

devtools::spell_check(pkg_dir)
  WORD              FOUND IN
anlyzing          description:3
contatining       simdesign.Rd:20
reproducability   import_nl.Rd:25

The biggest issue currently is the test coverage. How long do you think it will take to develop new tests? I can begin looking for reviewers but we'll really need the tests to be complete before they begin and it's best to contact reviewers when the package is ready for review. Let me know if you'd like me to put the review on hold or contact reviewers if you think you can address the issues swiftly.

@annakrystalli

This comment has been minimized.

Copy link
Collaborator

commented Oct 25, 2018

@nldoc @marcosci Apologies! I only just realised this was a pre-submission enquiry. So the package has been approved for review! 🎉 If you would like to go ahead, you already have the editors checks. Just let me know how you would like to proceed and sorry for jumping the gun!

@nldoc

This comment has been minimized.

Copy link
Author

commented Oct 25, 2018

Dear @annakrystalli and @sckott, thank you very much for your encouraging response and first feedback.
We are very happy about the chance to further improve our package. We already started fixing most of the issues you mentioned:

Editor checks:

* [x]  **Fit**: The package meets criteria for [fit](policies.md#fit) and [overlap](policies.md#fit)

* [ ]  **Automated tests:** Package has a testing suite and is tested via Travis-CI or another CI service.

* [x]  **License:** The package has a CRAN or OSI accepted license

* [x]  **Repository:** The repository link resolves correctly

* [ ]  **Archive** (JOSS only, may be post-review): The repository DOI resolves correctly

* [ ]  **Version** (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

Editor comments

Hello @nldoc 👋,

Thanks for your package submission! I look forward to learning more about it during the review. I've completed the editors checks and there's a few issues to clear before we proceed.

All tests and checks pass. However:

test coverage

17% test coverage is generally too low to be accepted currently and will likely need more tests. Particular attention is required on the scripts with 0% coverage. Is there are reason these scripts aren't currently being tested?

covr::package_coverage(pkg_dir)
nlrx Coverage: 17.55%
R/class_init.R: 0.00%
R/download_netlogo.R: 0.00%
R/export_nl.R: 0.00%
R/get_nl_spatial.R: 0.00%
R/import_nl.R: 0.00%
R/run_nl.R: 0.00%
R/simdesign_helper.R: 0.00%
R/util_eval.R: 0.00%
R/util_runnl_dyn.R: 0.00%
R/util_runnl.R: 0.00%
R/util_stuff.R: 0.00%
R/zzz.R: 0.00%
R/class_setget.R: 33.33%
R/analyze_nl.R: 94.02%
R/class_constr.R: 100.00%

We will develop tests for all functions (where possible) within the next two weeks. Unfortunately, some functions (especially those that call java via command line) can not be tested without providing an executable NetLogo version within the repository. However, including NetLogo in the package repository would need way too much space.

Good Practice
goodpractice::gp(pkg_dir)

It is good practice to:

* [ ]  not use "Depends" in DESCRIPTION, as it can
  cause name clashes, and poor interaction with other
  packages. Use "Imports" instead.

* [ ]  avoid long code lines, it is bad for
  readability. Also, many people prefer editor windows that
  are about 80 characters wide. Try make your lines shorter
  than 80 characters. Have a look at [styler](https://github.com/r-lib/styler) pkg.
  ```
  R/class_init.R:37:1
  R/class_init.R:38:1
  R/class_init.R:39:1
  R/class_init.R:40:1
  R/class_init.R:41:1
  ... and 98 more lines
  ```

* [ ]  avoid calling `setwd()`, it changes the global
  environment. If you need it, consider using `on.exit()` to
  restore the working directory.
  ```
  R/export_nl.R:39:3
  R/export_nl.R:40:11
  R/export_nl.R:48:3
  ```

* [ ]  not import packages as a whole, as this can
  cause name clashes between the imported packages. Instead,
  import only the specific functions you need.

* [ ]  fix this R CMD check WARNING: LaTeX errors when
  creating PDF version. This typically indicates Rd
  problems.
  ```
  LaTeX errors found: ! Paragraph ended before
  \Rd@code was complete. <to be read again> \par l.100 !
  Paragraph ended before \Rd@code was complete. <to be read
  again> \par l.125
  ```

* [ ]  avoid '`T`' and '`F`', as they are just variables
  which are set to the logicals `TRUE` and `FALSE` by
  default, but are not reserved words and hence can be
  overwritten by the user.  Hence, one should always use
  `TRUE` and `FALSE` for the logicals.
  ```
  R/analyze_nl.R:NA:NA
  R/analyze_nl.R:NA:NA
  ```

We solved most of these issues with our latest updates. The only issue that persists is the use of setwd() within the export_nl function. The function creates a zip file from a defined directory. If the zip file is created without setting the defined directory as working directory, the complete folder directory (back to the current working directory) is stored within the zip file. We did not find any other workaround beside setting the working directory via setwd(). If there are other solutions to create a proper zip file, we are happy to remove the setwd command. However, we followed the suggestion and added the on.exit() command to reset the working directory to the original directory.

spell check

I've pulled out a few typos that looked real. Might be worth running spell check to satisfy yourself too.

devtools::spell_check(pkg_dir)
  WORD              FOUND IN
anlyzing          description:3
contatining       simdesign.Rd:20
reproducability   import_nl.Rd:25

The biggest issue currently is the test coverage. How long do you think it will take to develop new tests? I can begin looking for reviewers but we'll really need the tests to be complete before they begin and it's best to contact reviewers when the package is ready for review. Let me know if you'd like me to put the review on hold or contact reviewers if you think you can address the issues swiftly.

As already mentioned, we will develop the remaining tests within the next two weeks. If there are no more issues than the ones stated above the package will be ready for review in two weeks at latest. Thus, we think it makes sense to already contact the reviewers.

@marcosci

This comment has been minimized.

Copy link

commented Oct 25, 2018

Thanks @annakrystalli for your efforts already 🎉

Regarding the tests:

NetLogo is ~200 Mb big, so, unfortunately, we can not deliver it with the package. This also means that we can not properly test if running NetLogo itself works.

Are you aware of a possibility to do that only on Travis (it could be skipped on CRAN then)?
This would mean that we somehow have to download NetLogo and unpack it before we build the package and test it.

@noamross

This comment has been minimized.

Copy link
Collaborator

commented Oct 25, 2018

I would encourage you to look at how rtika handles this: https://github.com/ropensci/rtika . The package provides a function to install the large tika .jar file, checks for it on startup, and provides an option for the user to specify a path to another installation if desired. Tests requiring the java jar (most of them) are skipped on CRAN but occur on Travis, with test scripts downloading the .jar file first.

@annakrystalli

This comment has been minimized.

Copy link
Collaborator

commented Oct 25, 2018

Thanks for your comments @noamross!

@nldoc, re: setwd() in export_nl

Have you tried supplying the path argument to list.files() and setting full.names = TRUE? This shoulh give you the full paths to the files you want to zip and not require you to set (or reset) the working directory.

eg I think you could do:

 utils::zip(zipfile=outfile, files = list.files(folder, full.names = TRUE), extras = "-r") 

I also notice you are using paste0() to concatenate file paths. It's better to use file.path() instead.

eg you could do

nltempfile <- file.path(nltempdir, "nlobject.rds")
@nldoc

This comment has been minimized.

Copy link
Author

commented Nov 8, 2018

Dear @annakrystalli,
we completed all the tasks suggested by you. We developed tests for most functions of the package. We also set up travis to run and execute NetLogo models, which allows us to test the main functions of the package. Additionally, we spent some time on increasing performance of the package. From our point of view at its current state the package is ready for review.

@annakrystalli

This comment has been minimized.

Copy link
Collaborator

commented Nov 9, 2018

Great! Thanks for your speedy efforts. I'll get on to the checks today and then contact reviewers.

@annakrystalli

This comment has been minimized.

Copy link
Collaborator

commented Nov 21, 2018

We have reviewers! 🎉

@nldoc could I ask that you add the review badge to your README?

[![](https://badges.ropensci.org/262_status.svg)](https://github.com/ropensci/onboarding/issues/262)

@marinapapa & @thomasp85 many thanks for agreeing to review! 🙏 Feel free to reach out with questions at any point.


Reviewers: @marinapapa @thomasp85
Due date: 2018-12-13

@marcosci

This comment has been minimized.

Copy link

commented Nov 22, 2018

I just added the review badge 👍 Very much looking forward to @marinapapa's & @thomasp85's comments, thank you for reviewing!

@sckott sckott changed the title Pre-submisstion Inquiry: nlrx nlrx Nov 26, 2018

@annakrystalli

This comment has been minimized.

Copy link
Collaborator

commented Dec 11, 2018


Reviewers: @marinapapa @thomasp85
New due date: 2018-12-20

@thomasp85

This comment has been minimized.

Copy link

commented Dec 18, 2018

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

  • A statement of need clearly stating problems the software is designed to solve and its target audience in README
  • Installation instructions: for the development version of package and any non-standard dependencies in README
  • Vignette(s) demonstrating major functionality that runs successfully locally
  • Function Documentation: for all exported functions in R help
  • Examples for all exported functions in R Help that run successfully locally
  • Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).
For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • A short summary describing the high-level functionality of the software
  • Authors: A list of authors with their affiliations
  • A statement of need clearly stating problems the software is designed to solve and its target audience.
  • References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

  • Installation: Installation succeeds as documented.
  • Functionality: Any functional claims of the software been confirmed.
  • Performance: Any performance claims of the software been confirmed.
  • Automated tests: Unit tests cover essential functions of the package
    and a reasonable range of inputs and conditions. All tests pass on the local machine.
  • Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Final approval (post-review)

  • The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing:


Review Comments

Documentation

I feel the README could be a bit more detailed when it comes to describing what the package does. As it stands, you need to already be familiar with NetLogo to understand the need/use for the package. While a detailed description of NetLogo is not suited for a README, a small section before Installation would make the README more welcome to new users of actor based simulations in general. Further it would be good to link to the NetLogo online ressources to help users with researching further information. The README should also clearly state that you need to install NetLogo manually and how this is done. Omission of this indicates that NetLogo is installed as part of the package installation.

The vignettes assumes the user is running on windows. While it may be impossible to create examples that run on all systems, as you have to point to the NetLogo installation it would be nice if the vignettes were structured in a way that made it easier to modify the code to your local system. This could e.g. be done by storing relevant paths in variables at the top of the vignette. I would like the Simdesing examples vignette to include more descriptive text to carry the reader through what is happening, and giving them an impediment to understand the steps. As it is now, it is mainly a collections of snippets to cope-paste (or at least it appears like that with the lack of motivation and discussion)

Functions are in general well-documented. I think it would make sense to put .initClasses and simdesign-class documentation as internal so it doesn't show up in the overview, as the docs are rather lacking (and not that important). All examples appear to be wrapped in \dontrun{}, which I'm pretty sure CRAN will take offense with. I can understand that running the actual simulations may be too heavy for an example, but a lot of the constructors and getters/setters are pretty benign and should be able to be run in examples

Functionality

As discussed above, the assumption that NetLogo is already installed is not clearly communicated. The download_netlogo() function should be highlighted in the README and "Getting Started" vignette. Apart from this, the installation processes as expected, and the package works as expected. It is clear that you have to have substantial understanding of NetLogo prior to using this package, but as this is also the intended audience it is not really a problem (some additional hand-holding from the documentation would be nice as discussed above). The performance of the package is mainly a function of the performance of NetLogo, and it does not appear that the package adds any unnecessary computational overhead.

The test coverage is generally good, and it is nice to see that they have made a strategy for testing functionality that can't be tested on CRAN

The package title needs to be in titlecase and the description shouldn't start with "The nlrx package..." (CRAN requirements, not mine). Apart from that the packaging guidelines seems to be followed (again, with the excdeption of the documentation discussed above)

@thomasp85

This comment has been minimized.

Copy link

commented Dec 18, 2018

I hope it is as it should - otherwise I’m open to a review of my review🙂

@annakrystalli

This comment has been minimized.

Copy link
Collaborator

commented Dec 18, 2018

I’m open to a review of my review🙂

😜❤️

All good, looks like pretty sound feedback to me! If @nldoc or @marcosci require any clarification I'm sure they'll reach out.

@marinapapa

This comment has been minimized.

Copy link

commented Dec 19, 2018

Package Review

Hi all, here is my review. I really like your package so I hope my comments will help to improve it :)

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

  • A statement of need clearly stating problems the software is designed to solve and its target audience in README
  • Installation instructions: for the development version of package and any non-standard dependencies in README
  • Vignette(s) demonstrating major functionality that runs successfully locally
  • Function Documentation: for all exported functions in R help
  • Examples for all exported functions in R Help that run successfully locally
  • Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Functionality

  • Installation: Installation succeeds as documented.
  • Functionality: Any functional claims of the software been confirmed.
  • Performance: Any performance claims of the software been confirmed.
  • Automated tests: Unit tests cover essential functions of the package
    and a reasonable range of inputs and conditions. All tests pass on the local machine.
  • Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Final approval (post-review)

  • The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 15


Review Comments

In general, I like the idea of the package and its functionalities. Some dependencies issues that were not stated made the review process a bit frustruating, but I think most of my points are very easy to tackle.

Installation & Documentation

  1. Installation worked in Windows, with the below problems:
  • R dependencies: R (>= 2.10) is mentioned in desciption, but two of the Imports packages (lhs and sf) need R >= 3.3. So nlrx could not be installed on a machine with older version.

  • Java: the package is using a java virtual mashine so Java needs to be installed on the local machine but is not mentioned anywhere. Installation is fine without it but of course the package will not function, the path set-up is needed in order to run.

So it would be helpful if the information for the above points were stated in the installation guidelines/ README/ description.

  1. A clearer statement of need is needed (as per rOpenSci guidelines), why the package is needed and should be preferred from just using Netlogo's behavioral space. Some of the text from the onboarding issue would fit nicely.

  2. README:

  • Netlogo dependencies: supported Netlogo versions are mentioned only in the download_netlogo function. This and the above prerequests from (1) should be included in README.

  • The first paragraph states what is the package doing in a very broad way. As mentioned above, a bit more emphasis can be given on stating the problems it is designed to solve and target audience.

  • Example: this session could be renamed as a Get started session and include the example. Also, starting with 'S4 classes' may be a bit too advanced for a student or a not-R-expert to understand, so I would restate the first paragraph in simpler words. The more techincal description is given at the DESCRIPTION file anyway, which I think is appropriate.

  1. Help files:

Given that NetLogo is a program widely used for teaching, I think a bit more detailed help files chould be of great importance:

  1. nl contructor: what are the experiment and simdesign should be explained more, or have a link to their help pages.
  2. experiment constructor: should be more detailed of what information the experiment holds (not only with example code).
  3. simdesign constructor: design types should be listed in the description, not just stated as examples, maybe also with links to the separate functions descriptions.
  4. analyze_nl:
  • typo: fullstop missing in value session.
  • this function is based on the simdesign, but given that the latter is stored in the nl object, it is not so obvious for a new user. Maybe it should be emphasized a bit more in the help file and include links to the separate analyze functions.
  • All the analyze_X functions have the same help file. If you think re-adding details of the analysis here (cause they exist in simdesign files) is not useful, you should at least add links to these helpfiles. Especially for:
  1. analyze_simple: is an empty function, but nothing is stated in its help file.
  2. simdesign_simple: even though constant parameterization is stated, the fact that no analysis is available in this design should be mentioned.
  3. run_nl_one and run_nl_all have the same title and description. A simple adjustment of 'Execute one NetLogo simulation' and 'Execute all NetLogo simulations' will do better.
  4. download_netlogo: the saving path is stated as: 'Character string with the name where the downloaded file is saved.'. I think it should be replaced by 'path to folder' as in the rest of help me files with parth as input.
  5. util_eval_: all functions help files are not very informative, in terms of what 'evaluate' means. However, the comments in the code of the functions are informative: 'check if there are any variables defined', so a simple copy paste should do.

Functions:

getstarted.Rmd

  • library(nlrx) should be called after the installation code

  • library(future) needs to be called before lines 100 and 102 can be implemented (otherwise operator %<-% and unquoted multisession are not recognized)

  • If Java path is not connected and the user uses "multisession" plan gets an unhelpful error for a non-existing csv file in Temp. For different plans, they get error for Java path missing, which helps. I don't know if you can add something to that, maybe not use 'multisession' in the getstarted?. But anyway not that important.

  • Attach an experiment: it should be mentioned if the user has to define all the model variables or the ones that are not defined will be assigned the default model's values.

nl@experiment:

  • When assigning variables lists: if you add parameters X and Y as contants as well you get an error: 'Columns X, Y must have unique names', which doesn't make lots of sense. If you know the problem it is clear that it means that the variables should not be defined as contants, but the error message is not helping the user see whats wrong. A message can be added.

  • If a parameter is wrong, after running the simulations, the results are going to be empty but no warning or other message will be shown before you try to assign in the results, and will get an error 'replacement has 1 row, data has 0'. It would be helful if there was a hint to the user to check the parameters in the experiment should be given.

spatial-output.Rmd

I couldn't get it to fully work. Two main problems:

  • line 86: get_nl_spatial runs but prints a very bad message, a repetiting (a lot of times) 'no non-missing arguments to min; returning Infno non-missing arguments to max; returning -Inf', which I don't know where it comes from. With another model without patches information the message does not appear.

  • line 137: for results_spatial_tibble, all metrics.turtles are returned NA. I think it comes from the full_join of ddplyr, which only preserves the grouping of x (so pathces) and the not matching values are ignored and replaced with NA. So the turtles cannot be plotted afterwards, getting error 'object 'turtles_x' not found'

And minor:

  • library(ggplot2) needs to be called earlier, before first plot of spatial output.
  • gganimate: also needs a renderer installed. I would mention or add the command for installing 'gifski' package (the default of gganimate), and also the 'png' package installed (which is needed by gganimate). Also, maybe add hashed-out the direct gganimate installation command (instead of just the link).

analyze_simple

Renaming to no_analysis may be helpful (a 'simple' analysis is still an analysis). So far if the user runs analyze_nl with simple design they get NULL, which can be confusing. A message can be added to be printed in analuze_simple in order to inform the user that 'no analysis can take place if a simple design is chosen'.

write_simoutput

  • Maybe a bit trivial, but when the 'out' folder is not created already, it would be helpful to add to the error message a hint that the user needs to create the folder indicated by the path.
  • Concerning the output directory, it would be handy for the user to be able to re-assign the output directory when the write function is being called. Maybe keep the default by the nl object, but if a new one is given as input by the user it would be used.

util_runnl.R:
There is some dead commented-out code at the end (lines 500 - 650).

load-model-parameters:
The name report-model-parameters would be more representative of this function.

General comments:

  • How does the package responds to runtime errors of the actual netlogo model? I think it inputs the error message in the object and doesn't give any warnings. Maybe you can try to catch some main runtime errors of netlogo and print a warning. Aditionally, if the netlogo model is broken the results will run forever (or at least as much patience I had :P ); maybe you could add something to break the function and show a warning message to check the netlogo model. Or state that the package will not respond to problems in netlogo, so the user can always have this in mind.

  • I didn't find it easy to visualize models of collective motion where the heading of individuals is an important aspect. A plotting example of schooling, for translating netlogo's heading metric system to ggplot/gganimate for individual points would be a very valuable addition.

In total, I think it can be a very useful package. As I mentioned above, the package can gain a lot from improving its documentation. Also of course the variety of Netlogo models is very large so also by including another example of a quite different model may add value to it.

@marinapapa

This comment has been minimized.

Copy link

commented Dec 19, 2018

And as @thomasp85 said, I m happy to review my review if needed 😊

@nldoc

This comment has been minimized.

Copy link
Author

commented Dec 19, 2018

Thank you very much for your detailed reviews @thomasp85 and @marinapapa
Your comments are very helpful for improving the package functionality and documentation.
The issues you mentioned seem straightforward to us. We will immediately start working on improvements and come back to you with a detailed response to your comments.

@annakrystalli

This comment has been minimized.

Copy link
Collaborator

commented Dec 19, 2018

Indeed thanks so much for the thorough review @marinapapa!

@nldoc & @marcosci, over to you now. We normally ask to aim for responses to reviewers within two weeks but with the holidays coming, it is quite alright if you take some extra time over it.

So wishing all in the thread Happy Holidays and enjoyable downtime! 🌟🎄🎅

@marcosci

This comment has been minimized.

Copy link

commented Dec 19, 2018

Hi Anna, Marina and Thomas - thanks a ton for the very nice reviews!

Just trying to fix the issues @marinapapa had with the get_nl_spatial function - can you provide me with the script you had that caused the errors? :)
Just tried it with our toy examples (https://github.com/nldoc/nldoc_playground) and I couldn't reproduce that.

Edit: I figured it out - the no non-missing arguments to min; returning Infno non-missing arguments to max; returning -Inf warning only appears in Rmarkdown blocks, not in normal R scripts - I could not figure out how to solve that, but it doesn't see to be too urgent, or?

library(nlrx)

nl <- nl(nlversion = "6.0.4",
         nlpath = "/home/marco/Documents/rpackages/nlrx_usecase/1_Helper/NetLogo 6.0.4/",
         modelpath = "/home/marco/Documents/rpackages/nlrx_usecase/1_Helper/NetLogo 6.0.4/app/models/Sample Models/Biology/Wolf Sheep Predation.nlogo",
         jvmmem = 1024)

nl@experiment <- experiment(expname = "nlrx_spatial",
                            outpath="out/",
                            repetition = 1,      
                            tickmetrics = "true",
                            idsetup = "setup",   
                            idgo = "go",         
                            idfinal = NA_character_,  
                            idrunnum = NA_character_,
                            runtime = 100,
                            evalticks = seq(1,100),
                            metrics = c("count sheep","count wolves"),
                            metrics.turtles = c("who", "pxcor", "pycor", "breed"),
                            metrics.patches = c("pxcor", "pycor", "pcolor"),
                            constants = list("model-version" = "\"sheep-wolves-grass\"",
                                             'initial-number-sheep' = 100,
                                             'initial-number-wolves' = 50,
                                             "grass-regrowth-time" = 30,
                                             "sheep-gain-from-food" = 4,
                                             "wolf-gain-from-food" = 20,
                                             "sheep-reproduce" = 4,
                                             "wolf-reproduce" = 5,
                                             "show-energy?" = "false")
)

# Attach simdesign simple using only constants
nl@simdesign <- simdesign_simple(nl=nl,
                                 nseeds=1)
#> Creating simple simulation design

# Run simulations and store output in results
results <- run_nl_all(nl = nl, cleanup = "all")

# Attach results to nl object:
setsim(nl, "simoutput") <- results

# Report spatial data:
results_spatial <- get_nl_spatial(nl,
                                  turtles = TRUE,
                                  patches = TRUE,
                                  turtle_coords = "px",
                                  format="spatial")
#> Joining, by = c("[run number]", "model-version", "initial-number-sheep", "initial-number-wolves", "grass-regrowth-time", "sheep-gain-from-food", "wolf-gain-from-food", "sheep-reproduce", "wolf-reproduce", "show-energy?", "random-seed", "[step]", "count sheep", "count wolves", "siminputrow")

devtools::session_info()
#> ─ Session info ──────────────────────────────────────────────────────────
#>  setting  value                       
#>  version  R version 3.5.1 (2018-07-02)
#>  os       Ubuntu 18.10                
#>  system   x86_64, linux-gnu           
#>  ui       X11                         
#>  language en_US                       
#>  collate  en_US.UTF-8                 
#>  ctype    en_US.UTF-8                 
#>  tz       America/North_Dakota/Beulah 
#>  date     2018-12-19                  
#> 
#> ─ Packages ──────────────────────────────────────────────────────────────
#>  package     * version   date       lib source                      
#>  assertthat    0.2.0     2017-04-11 [1] CRAN (R 3.5.0)              
#>  backports     1.1.2     2017-12-13 [1] CRAN (R 3.5.0)              
#>  bindr         0.1.1     2018-03-13 [1] CRAN (R 3.5.0)              
#>  bindrcpp    * 0.2.2     2018-03-29 [1] CRAN (R 3.5.0)              
#>  callr         3.1.0     2018-12-10 [1] CRAN (R 3.5.1)              
#>  class         7.3-14    2015-08-30 [4] CRAN (R 3.5.0)              
#>  classInt      0.2-3     2018-04-16 [1] CRAN (R 3.5.0)              
#>  cli           1.0.1     2018-09-25 [1] CRAN (R 3.5.1)              
#>  codetools     0.2-15    2016-10-05 [4] CRAN (R 3.5.0)              
#>  crayon        1.3.4     2017-09-16 [1] CRAN (R 3.5.0)              
#>  DBI           1.0.0     2018-05-02 [1] CRAN (R 3.5.0)              
#>  desc          1.2.0     2018-05-01 [1] CRAN (R 3.5.0)              
#>  devtools      2.0.1     2018-10-26 [1] CRAN (R 3.5.1)              
#>  digest        0.6.18    2018-10-10 [1] CRAN (R 3.5.1)              
#>  dplyr         0.7.8     2018-11-10 [1] CRAN (R 3.5.1)              
#>  e1071         1.7-0     2018-07-28 [1] CRAN (R 3.5.1)              
#>  evaluate      0.12      2018-10-09 [1] CRAN (R 3.5.1)              
#>  fs            1.2.6     2018-08-23 [1] CRAN (R 3.5.1)              
#>  furrr         0.1.0     2018-05-16 [1] CRAN (R 3.5.0)              
#>  future        1.10.0    2018-10-17 [1] CRAN (R 3.5.1)              
#>  globals       0.12.4    2018-10-11 [1] CRAN (R 3.5.1)              
#>  glue          1.3.0     2018-10-29 [1] github (tidyverse/glue)     
#>  highr         0.7       2018-06-09 [1] CRAN (R 3.5.0)              
#>  hms           0.4.2     2018-03-10 [1] CRAN (R 3.5.0)              
#>  htmltools     0.3.6     2017-04-28 [1] CRAN (R 3.5.0)              
#>  knitr         1.21      2018-12-10 [1] CRAN (R 3.5.1)              
#>  lattice       0.20-38   2018-11-04 [4] CRAN (R 3.5.1)              
#>  listenv       0.7.0     2018-01-21 [1] CRAN (R 3.5.0)              
#>  magrittr      1.5       2014-11-22 [1] CRAN (R 3.5.0)              
#>  memoise       1.1.0     2017-04-21 [1] CRAN (R 3.5.0)              
#>  nlrx        * 0.1.0     2018-12-19 [1] local                       
#>  pillar        1.3.0     2018-07-14 [1] CRAN (R 3.5.1)              
#>  pkgbuild      1.0.2     2018-10-16 [1] CRAN (R 3.5.1)              
#>  pkgconfig     2.0.2     2018-08-16 [1] CRAN (R 3.5.1)              
#>  pkgload       1.0.2     2018-10-29 [1] CRAN (R 3.5.1)              
#>  prettyunits   1.0.2     2015-07-13 [1] CRAN (R 3.5.0)              
#>  processx      3.2.1     2018-12-05 [1] CRAN (R 3.5.1)              
#>  ps            1.2.1     2018-11-06 [1] CRAN (R 3.5.1)              
#>  purrr       * 0.2.5     2018-05-29 [1] CRAN (R 3.5.0)              
#>  R6            2.3.0     2018-10-04 [1] CRAN (R 3.5.1)              
#>  raster        2.8-4     2018-11-03 [1] CRAN (R 3.5.1)              
#>  Rcpp          1.0.0     2018-11-07 [1] CRAN (R 3.5.1)              
#>  readr         1.3.0     2018-12-11 [1] CRAN (R 3.5.1)              
#>  remotes       2.0.2     2018-10-30 [1] CRAN (R 3.5.1)              
#>  rlang         0.3.0.1   2018-10-25 [1] CRAN (R 3.5.1)              
#>  rmarkdown     1.11      2018-12-08 [1] CRAN (R 3.5.1)              
#>  rprojroot     1.3-2     2018-01-03 [1] CRAN (R 3.5.0)              
#>  sessioninfo   1.1.1     2018-11-05 [1] CRAN (R 3.5.1)              
#>  sf            0.7-1     2018-10-24 [1] CRAN (R 3.5.1)              
#>  sp            1.3-1     2018-06-05 [1] CRAN (R 3.5.0)              
#>  spData        0.2.9.6   2018-12-03 [1] CRAN (R 3.5.1)              
#>  spDataLarge   0.2.7.1   2018-09-16 [1] github (Nowosad/spDataLarge)
#>  stringi       1.2.4     2018-07-20 [1] CRAN (R 3.5.1)              
#>  stringr       1.3.1     2018-05-10 [1] CRAN (R 3.5.0)              
#>  testthat      2.0.1     2018-10-13 [1] CRAN (R 3.5.1)              
#>  tibble        1.4.2     2018-01-22 [1] CRAN (R 3.5.0)              
#>  tidyr         0.8.2     2018-10-28 [1] CRAN (R 3.5.1)              
#>  tidyselect    0.2.5     2018-10-11 [1] CRAN (R 3.5.1)              
#>  units         0.6-2     2018-12-05 [1] CRAN (R 3.5.1)              
#>  usethis       1.4.0     2018-08-14 [1] CRAN (R 3.5.1)              
#>  withr         2.1.2     2018-03-15 [1] CRAN (R 3.5.0)              
#>  xfun          0.4       2018-10-23 [1] CRAN (R 3.5.1)              
#>  XML           3.98-1.16 2018-08-19 [1] CRAN (R 3.5.1)              
#>  yaml          2.2.0     2018-07-25 [1] CRAN (R 3.5.1)              
#> 
#> [1] /home/marco/R/x86_64-pc-linux-gnu-library/3.5
#> [2] /usr/local/lib/R/site-library
#> [3] /usr/lib/R/site-library
#> [4] /usr/lib/R/library

Created on 2018-12-19 by the reprex package (v0.2.1)

@annakrystalli

This comment has been minimized.

Copy link
Collaborator

commented Jan 5, 2019

Hello all and happy (belated) New Year! 💫

Just checking in to see how things are going.

I also asked the editors about your query regarding the repeating warning message when running get_nl_spatial in Rmd. While not necessarily more urgent than other issues,it still needs to be addressed prior to approval.

@noamross also provided some useful tips on how to troubleshoot and then set up a test to monitor the behaviour:

Debugging this thing can be tricky, but a good place to start is to make the minimal Rmd that produces and render it using a script with rmarkdown::render(). Then debug(), options(error=recover), etc are all available to you., and then you can write a test to check that it renders properly in the future.

Let me know if you have any further questions!

@nldoc

This comment has been minimized.

Copy link
Author

commented Jan 5, 2019

Happy New Year to you too!

Let me give you a short update on the progress of our revisions.
We revised large parts of the package documentation and already resolved most of the issues mentioned by the reviewers. There are still some small things to be done (for example the get_nl_spatial warning messages; thank you @annakrystalli for your hints, we will try to solve this issue asap).
We will continue fixing the remaining issues on Monday and I think we are ready to post our detailed response to all reviewer comments by midweek.
Have a nice weekend!

@annakrystalli

This comment has been minimized.

Copy link
Collaborator

commented Jan 5, 2019

Thanks for the updates @nldoc! 👍

Have a nice weekend too. 😊

@marcosci

This comment has been minimized.

Copy link

commented Jan 9, 2019

Detailed response to reviewers

Dear @marinapapa and @thomasp85,
thank you very much for your helpful and constructive feedback on our R package.
We agree with all the issues you mentioned and we revised our code and package documentation accordingly. The following sections will give you a detailed overview of our revisions, pointing to specific commits, where possible and links to the corresponding vignette/article sections of the corresponding package homepage.

Reviewer comments by @thomasp85

Documentation

I feel the README could be a bit more detailed when it comes to describing what the package does. As it stands, you need to already be familiar with NetLogo to understand the need/use for the package. While a detailed description of NetLogo is not suited for a README, a small section before Installation would make the README more welcome to new users of actor based simulations in general. Further it would be good to link to the NetLogo online ressources to help users with researching further information. The README should also clearly state that you need to install NetLogo manually and how this is done. Omission of this indicates that NetLogo is installed as part of the package installation.

  • We revised the README and added more specific information (see README update commit 1 and following, see also home site of package homepage, built from README.Rmd)
  • More specifically, we added more details on the purpose and usage of our package. We also added general information on NetLogo and links to NetLogo online resources.
  • We added a Prerequirements section with installation guides and notes on NetLogo and Java

The vignettes assumes the user is running on windows. While it may be impossible to create examples that run on all systems, as you have to point to the NetLogo installation it would be nice if the vignettes were structured in a way that made it easier to modify the code to your local system. This could e.g. be done by storing relevant paths in variables at the top of the vignette.

I would like the Simdesing examples vignette to include more descriptive text to carry the reader through what is happening, and giving them an impediment to understand the steps. As it is now, it is mainly a collections of snippets to cope-paste (or at least it appears like that with the lack of motivation and discussion)

  • The revised simdesign examples vignette contains more detailed information on the simdesign methods and how they can be used for model analysis. However, we only included very brief descriptions of the methods and linked to the relevant publications/R-packages for more detailed information on specific methods.

Functions are in general well-documented. I think it would make sense to put .initClasses and simdesign-class documentation as internal so it doesn't show up in the overview, as the docs are rather lacking (and not that important).

  • The class initialization function is now set as internal and does not show up in the functions overview table.

All examples appear to be wrapped in \dontrun{}, which I'm pretty sure CRAN will take offense with. I can understand that running the actual simulations may be too heavy for an example, but a lot of the constructors and getters/setters are pretty benign and should be able to be run in examples

  • The \dontrun{} wrappers have been removed where possible (see commit)

Functionality

As discussed above, the assumption that NetLogo is already installed is not clearly communicated. The download_netlogo() function should be highlighted in the README and "Getting Started" vignette. Apart from this, the installation processes as expected, and the package works as expected. It is clear that you have to have substantial understanding of NetLogo prior to using this package, but as this is also the intended audience it is not really a problem (some additional hand-holding from the documentation would be nice as discussed above). The performance of the package is mainly a function of the performance of NetLogo, and it does not appear that the package adds any unnecessary computational overhead.

  • As stated above, we completely revised the README and getstarted vignette accordingly and added more detailed information on NetLogo and the installation process. The download_netlogo() function is now also mentioned in our new Prerequirements section.

The test coverage is generally good, and it is nice to see that they have made a strategy for testing functionality that can't be tested on CRAN

The package title needs to be in titlecase and the description shouldn't start with "The nlrx package..." (CRAN requirements, not mine). Apart from that the packaging guidelines seems to be followed (again, with the excdeption of the documentation discussed above)

  • We adjusted the title and the description accordingly.

Reviewer comments by @marinapapa

In general, I like the idea of the package and its functionalities. Some dependencies issues that were not stated made the review process a bit frustruating, but I think most of my points are very easy to tackle.

Installation & Documentation

  1. Installation worked in Windows, with the below problems:
    • R dependencies: R (>= 2.10) is mentioned in desciption, but two of the Imports packages (lhs and sf) need R >= 3.3. So nlrx could not be installed on a machine with older version.
    • Java: the package is using a java virtual mashine so Java needs to be installed on the local machine but is not mentioned anywhere. Installation is fine without it but of course the package will not function, the path set-up is needed in order to run.
      So it would be helpful if the information for the above points were stated in the installation guidelines/ README/ description.
  • We adjusted the R-dependency information in the description file accordingly
  • We revised our README and get started vignette and added a new section on Prerequirements that contains information on how to download and install NetLogo and instructions for Java installation.
  1. A clearer statement of need is needed (as per rOpenSci guidelines), why the package is needed and should be preferred from just using Netlogo's behavioral space. Some of the text from the onboarding issue would fit nicely.
  • Both, the description and the first sections of the README now contain more specific information on the purpose of the nlrx package. For instance, we now highlight the higher flexibility in designing experiments compared to NetLogo Behavior Space and the reproducible workflow.
  1. README:
  • Netlogo dependencies: supported Netlogo versions are mentioned only in the download_netlogo function. This and the above prerequests from (1) should be included in README.
  • We added information on the supported NetLogo versions (>=5.3.1) to the new Prerequirements section of the README and get started vignette.
  • The first paragraph states what is the package doing in a very broad way. As mentioned above, a bit more emphasis can be given on stating the problems it is designed to solve and target audience.
  • As mentioned above, the introduction section of the README now contains more information on the purpose and main features of the package.
  • Example: this session could be renamed as a Get started session and include the example. Also, starting with 'S4 classes' may be a bit too advanced for a student or a not-R-expert to understand, so I would restate the first paragraph in simpler words. The more techincal description is given at the DESCRIPTION file anyway, which I think is appropriate.
  • As suggested, we renamed the example section to Get started
  • We tried to explain the class concept in a less technical way. However, we think it is important to make the nested class concept of the nlrx package clear from the beginning.
  1. Help files:
    Given that NetLogo is a program widely used for teaching, I think a bit more detailed help files chould be of great importance:
  2. nl contructor: what are the experiment and simdesign should be explained more, or have a link to their help pages.
  • We added information on experiment and simdesign classes to the nl helpfile including links to the corresponding documentation pages.
  1. experiment constructor: should be more detailed of what information the experiment holds (not only with example code).
  • We added more detailed information on the type of information that is entered into each experiment slot to the experiment helpfile.
  1. simdesign constructor: design types should be listed in the description, not just stated as examples, maybe also with links to the separate functions descriptions.
  • We added information on all supported simdesign types and helper functions to the simdesign helpfile. We also added links to the corresponding helper function ducoumentation pages.
  1. analyze_nl:
  • typo: fullstop missing in value session.
  • revised
  • this function is based on the simdesign, but given that the latter is stored in the nl object, it is not so obvious for a new user. Maybe it should be emphasized a bit more in the help file and include links to the separate analyze functions.
  • We added information on the different analyze_nl subfunctions to the analyze_nl helpfile. We also added links to the corresponding help files of the analysis functions.
  • All the analyze_X functions have the same help file. If you think re-adding details of the analysis here (cause they exist in simdesign files) is not useful, you should at least add links to these helpfiles. Especially for:
  • We added an overview list of analyze_X functions with brief explanations to the analzye_nl helpfile. We also included links to the corresponding packages that are used for index calculations. We also added the brief explanation from this overview list to each analyze_X function helpfile.
  1. analyze_simple: is an empty function, but nothing is stated in its help file.
  • We removed all references to analyze_simple from the documentation as for now, no general useful postprocessing analysis can be done for just one simulation run. However, we kept the skeleton of the function in order to print a warning message if a nl object with a simple simdesign is parsed to the analyze_nl function.
  • Additionally, we added a list of simdesign without postprocessing function to the [analyze_nl help file](
  • https://nldoc.github.io/nlrx/reference/analyze_nl.html)
  1. simdesign_simple: even though constant parameterization is stated, the fact that no analysis is available in this design should be mentioned.
  1. run_nl_one and run_nl_all have the same title and description. A simple adjustment of 'Execute one NetLogo simulation' and 'Execute all NetLogo simulations' will do better.
  1. download_netlogo: the saving path is stated as: 'Character string with the name where the downloaded file is saved.'. I think it should be replaced by 'path to folder' as in the rest of help me files with parth as input.
  1. util_eval_: all functions help files are not very informative, in terms of what 'evaluate' means. However, the comments in the code of the functions are informative: 'check if there are any variables defined', so a simple copy paste should do.
  • We adjusted the documentation of the evaluation functions accordingly (see commit)

Functions:

getstarted.Rmd

  • library(nlrx) should be called after the installation code
  • All code example section in the README and vignette files now contain a call of library(nlrx)
  • library(future) needs to be called before lines 100 and 102 can be implemented (otherwise operator %<-% and unquoted multisession are not recognized)
  • We removed the future operator from the basic examples in the README and GetStarted vignette for simplification. The usage of future in combination with nlrx is now explained within the Advanced Configuration vignette, including a call to load the future package (library(future)).
  • If Java path is not connected and the user uses "multisession" plan gets an unhelpful error for a non-existing csv file in Temp. For different plans, they get error for Java path missing, which helps. I don't know if you can add something to that, maybe not use 'multisession' in the getstarted?. But anyway not that important.
  • As stated above, we removed usage of future from the README and GetStarted vignette for simplification.
  • Attach an experiment: it should be mentioned if the user has to define all the model variables or the ones that are not defined will be assigned the default model's values.

nl@experiment:

  • When assigning variables lists: if you add parameters X and Y as contants as well you get an error: 'Columns X, Y must have unique names', which doesn't make lots of sense. If you know the problem it is clear that it means that the variables should not be defined as contants, but the error message is not helping the user see whats wrong. A message can be added.
  • We added an error message that is thrown when a simdesign is attached to a nl object with an experiment that has the same NetLogo parameter in the variables and constants list. The same message appears if such a nl object is commited to eval_variables_constants(). We also added a reference to this evaluation function to the Advanced configuration vignette.
  • If a parameter is wrong, after running the simulations, the results are going to be empty but no warning or other message will be shown before you try to assign in the results, and will get an error 'replacement has 1 row, data has 0'. It would be helful if there was a hint to the user to check the parameters in the experiment should be given.
  • We added two warning messages to util_gather_results: (1) When using the systems default temp files directory, on linux systems it can happen that temporary files are deleted by system processes. If no csv file has been found for the current simulation, a warning message will appear, stating that the temporary files directory may need to be reassigned to another directory. (2) if the model output file is empty, a warning message will appear, stating that parameter definitions might be invalid or a runtime error might have occured.

spatial-output.Rmd
I couldn't get it to fully work. Two main problems:

  • line 86: get_nl_spatial runs but prints a very bad message, a repetiting (a lot of times) 'no non-missing arguments to min; returning Infno non-missing arguments to max; returning -Inf', which I don't know where it comes from. With another model without patches information the message does not appear.
  • We attached a .Rmd (nlrx_spatial_debug.zip) in which we tried to reproduce the warnings. With the current version of nlrx, it does not seem to produce these messages anymore. Can you confirm that?
  • line 137: for results_spatial_tibble, all metrics.turtles are returned NA. I think it comes from the full_join of ddplyr, which only preserves the grouping of x (so pathces) and the not matching values are ignored and replaced with NA. So the turtles cannot be plotted afterwards, getting error 'object 'turtles_x' not found'
  • The error was human induced - we just missed to update the vignette to the current version of nlrx. We fixed this now, turtles_x is not returned anymore. This is due to different ways agent coordinates can be returned, either x or pc. nlrx now returns this as column name.

And minor:

  • library(ggplot2) needs to be called earlier, before first plot of spatial output.
  • gganimate: also needs a renderer installed. I would mention or add the command for installing 'gifski' package (the default of gganimate), and also the 'png' package installed (which is needed by gganimate). Also, maybe add hashed-out the direct gganimate installation command (instead of just the link).

analyze_simple
Renaming to no_analysis may be helpful (a 'simple' analysis is still an analysis). So far if the user runs analyze_nl with simple design they get NULL, which can be confusing. A message can be added to be printed in analuze_simple in order to inform the user that 'no analysis can take place if a simple design is chosen'.

  • We added a print message to analyze_simple stating that no analysis is implemented yet for simdesign_simple.

write_simoutput

  • Maybe a bit trivial, but when the 'out' folder is not created already, it would be helpful to add to the error message a hint that the user needs to create the folder indicated by the path.
  • write_simoutput will now print an error message if the chosen directory cannot be found.
  • Concerning the output directory, it would be handy for the user to be able to re-assign the output directory when the write function is being called. Maybe keep the default by the nl object, but if a new one is given as input by the user it would be used.
  • It is now possible to redirect the output to a user-defined directory using the outpath parameter of the write_simoutput function.

util_runnl.R:
There is some dead commented-out code at the end (lines 500 - 650).

  • This code section has been removed.

load-model-parameters:
The name report-model-parameters would be more representative of this function.

  • We renamed the load-model-parameters function to report-model-parameters.

General comments:

  • How does the package responds to runtime errors of the actual netlogo model? I think it inputs the error message in the object and doesn't give any warnings. Maybe you can try to catch some main runtime errors of netlogo and print a warning. Aditionally, if the netlogo model is broken the results will run forever (or at least as much patience I had :P ); maybe you could add something to break the function and show a warning message to check the netlogo model. Or state that the package will not respond to problems in netlogo, so the user can always have this in mind.
  • Thats right, usually if the running NetLogo model encounters a runtime error the error message is printed in the R console. It can also happen that a model simulation freezes due to Java runtime errors. Unfortunately it is not possible to terminate the Java virtual machine or print an error message to the console after such a runtime error occured. To our knowledge the only way to terminate such a freezed JVM is to execute system.exit() which would also terminate the current R-session. However, we improved our documentation and added two new sections to the Advanced configuration vignette.
    The Handling NetLogo runtime errors section gives information on runtime errors and hints for debugging.
    The Capturing progress of model simulations section includes information on how to capture progress of running simulations which might help for debugging and identifying freezed model simulations. The section gives several examples on how simulation progress can be monitored.
  • I didn't find it easy to visualize models of collective motion where the heading of individuals is an important aspect. A plotting example of schooling, for translating netlogo's heading metric system to ggplot/gganimate for individual points would be a very valuable addition.

In total, I think it can be a very useful package. As I mentioned above, the package can gain a lot from improving its documentation. Also of course the variety of Netlogo models is very large so also by including another example of a quite different model may add value to it.

  • We are confident that our revisions improved the package documentation a lot. We hope our changes are satisfying for you, too. In line with your suggestion to add more examples for different model types we also added an example of the Preferential Attachment model to show how link metrics can be measured and plotted using either ggplot or the igraph package.

Cheers,
Jan and Marco

@annakrystalli

This comment has been minimized.

Copy link
Collaborator

commented Jan 10, 2019

Thank you @marcosci & @nldoc for the detailed response and improvements!

@thomasp85 & @marinapapa , over to you. Please feedback on whether the responses have adequately addressed the issues you've raised. Feel free to reach out for any clarification.

@marinapapa

This comment has been minimized.

Copy link

commented Jan 13, 2019

Dear Jan and Marco,

I am sorry for not responding to your initial message about get_nl_spatial . It is indeed running smoothly now so no need for anything else :)

The new documentation is great and all my initial issues are adequately addressed.

I also like very very much the interfacing-different-models addition! Some comments:

  • A typo: on the fire model example (line 130) you are inputting the diffusion model again.
  • Ants example:
  1. scale_color_gradient_tableau (line 267) needs library(ggthemes)
  2. animate(line 275) throws error: 'time data must be the same class in all layers'
  • Waves example: line 322 run_nl_all gives me a message
    "you must specify either --setup-file or --experiment (or both)
    'Physics' is not recognized as an internal or external command,
    operable program or batch file. " followed by an error:
    "Error in util_gather_results(nl, outfile, seed, siminputrow) : Temporary output file C:..\Temp\RtmpCIfUkx\nlrx7380_125bc5e046c5f.csvnot found. On unix systems this can happen if the default system temp folder is used. Try reassigning the default temp folder for this R session (unixtools package)."

I didn't try much to solve the errors, maybe it is something very straight forward. Everything else works nicely :)

@marcosci

This comment has been minimized.

Copy link

commented Jan 14, 2019

Hi Marina,

thanks again for the comments!
We fixed all the issues you mentioned in the vignette - the wave example was mainly due to OS differences between Jan and myself.

Cheers,
Marco

PS: If you ever use gganimate for real flocking data or more complex models, please let me know :) would love to see that!

@thomasp85

This comment has been minimized.

Copy link

commented Jan 14, 2019

Hi Jan and Marco

I can only echo Marina in saying that all raised issues seems to have been solved. You have gone all-in on providing good documentation and guidance which warms my heart :-)

I do not have any additional comments to the package as it appears now.

best
Thomas

@annakrystalli

This comment has been minimized.

Copy link
Collaborator

commented Jan 15, 2019

Approved! 🚀

Many many thanks @nldoc & @marcosci for submitting and being so responsive and @marinapapa & @thomasp85 for your most thorough and helpful reviews! 😺

To-dos:

  • Transfer the repo to rOpenSci's "ropensci" GitHub organization under "Settings" in your repo. I have invited you to a team that should allow you to do so. You'll be made admin once you do.
  • Add the rOpenSci footer to the bottom of your README
    " [![ropensci_footer](https://ropensci.org/public_images/ropensci_footer.png)](https://ropensci.org)"
  • Fix any links in badges for CI and coverage to point to the ropensci URL. We no longer transfer Appveyor projects to ropensci Appveyor account so after transfer of your repo to rOpenSci's "ropensci" GitHub organization the badge should be [![AppVeyor Build Status](https://ci.appveyor.com/api/projects/status/github/ropensci/nlrx?branch=master&svg=true)](https://ci.appveyor.com/project/nldoc/nlrx).
  • We're starting to roll out software metadata files to all ropensci packages via the Codemeta initiative, see https://github.com/ropensci/codemetar/#codemetar for how to include it in your package, after installing the package - should be easy as running codemetar::write_codemeta() in the root of your package.

Should you want to aknowledge your reviewers in your package DESCRIPTION, you can do so by making them "rev"-type contributors in the Authors@R field (with their consent). More info on this here.

Welcome aboard! We'd also love a blog post about your package, either a short-form intro to it (https://ropensci.org/tech-notes/) or long-form post with more narrative about its development. (https://ropensci.org/blog/). If you are interested, @stefaniebutland will be in touch about content and timing.

We've started putting together a gitbook with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here.

Feel free to submit to MEE next. 😁

@annakrystalli

This comment has been minimized.

Copy link
Collaborator

commented Jan 15, 2019

Also, @marinapapa & @thomasp85, would you mind checking any remaining check boxes in your review comment to indicate formal acceptance? Many thanks again!

@nldoc

This comment has been minimized.

Copy link
Author

commented Jan 16, 2019

Many thanks to you, @annakrystalli, @marinapapa, @thomasp85 and the whole rOpenSci community. We are very glad that nlrx is now a part of it 😃.

We already took care of the listed To-dos. However, the link to our webpage at the top of the repository page is still pointing to the nldoc repository and it seems I don't have the user rights to edit the link (I don't see the edit button). We would also like to add some useful tags to the header of the repository. It would be great if you could set the user rights for me or @marcosci.

@marinapapa and @thomasp85, if you don`t mind we would like to add you as reviewers in our package Description file. Are you fine with that?

@annakrystalli

This comment has been minimized.

Copy link
Collaborator

commented Jan 16, 2019

Thanks @nldoc!

I've just returned admin rights to yourself and @marcosci 😁

@annakrystalli

This comment has been minimized.

Copy link
Collaborator

commented Jan 16, 2019

One last thing, I see you are using lifecycle badges (👍). However, at the minute you've got it set to Experimental which is defined as:

An experimental package is in the very early stages of development. The API will be changing frequently as we rapidly iterate and explore variations in search of the best fit. Experimental packages will make API breaking changes without deprecation, so you are generally best off waiting until the package is more mature before you use it. Experimental packages will not be released on CRAN

We'd expect an rOpenSci package to be at the very list at maturing stage:

The API of a maturing package has been roughed out, but finer details likely to change. Once released to CRAN, we will strive to maintain backward compatibility, but the package needs wider usage in order to get more feedback and find the optimal API

Could you please confirm that you consider it to be at this stage? And if so could you please change the lifecycle to maturing?

@marcosci

This comment has been minimized.

Copy link

commented Jan 16, 2019

@annakrystalli that was just a remnant of the beginning of the package - I updated the badge, as we consider it quite stable now 👍

@annakrystalli

This comment has been minimized.

Copy link
Collaborator

commented Jan 16, 2019

Thanks @marcosci .

Well, that's a wrap! 😺 Thanks again all 👋

@marinapapa

This comment has been minimized.

Copy link

commented Jan 17, 2019

@nldoc more than fine with adding me to your Description file 😊 Thanks

@marcosci

This comment has been minimized.

Copy link

commented Jan 21, 2019

Did that, thanks again!

@stefaniebutland

This comment has been minimized.

Copy link

commented Jan 22, 2019

Hello @nldoc and @marcosci and congratulations on passing peer review! Another one for you Marco.

Are you interested in writing a blog post or tech note? This link will give you many examples of blog posts by authors of peer-reviewed packages so you can get an idea of the style and length you prefer: https://ropensci.org/tags/software-peer-review/.

Technotes are here: https://ropensci.org/technotes/.

Here are some technical and editorial guidelines for contributing a post: https://github.com/ropensci/roweb2#contributing-a-blog-post.

Happy to answer any questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.