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 #156

Draft
wants to merge 27 commits into
base: main
Choose a base branch
from
Draft

JOSS paper #156

wants to merge 27 commits into from

Conversation

jpdunc23
Copy link
Contributor

@jpdunc23 jpdunc23 commented Apr 20, 2023

Do not merge.

@tiffanymtang
Copy link
Collaborator

Thanks so much, James! Great first draft! Some comments below. Feel free to ignore if you think it's beyond the scope of this paper or unnecessary.

In the code chunk,

  • some comments could be helpful to explain what various parts of the code are doing, e.g., "instantiating the DGP", "combine DGP, Method, Evaluator, Visualizer into an Experiment", "vary across parameters", "document"
  • in the create_*(), would it be helpful to write out the argument names so that fig1 can refer to those argument names too (e.g., dgp_fun)? May also be helpful to add a comment that says where to put additional arguments that get fed into dgp_fun (e.g., sd)
  • should we add_evaluator and add_visualizer in the experiment? Otherwise, eval and viz aren't actually involved in the experiment and the docs won't show any results
  • maybe use add_dgp() instead of dgp_list for consistency and easier understanding of our grammar?
  • change dgp2 vary across param from sparse to vary across dgp1 and sd?

In the main text,

  • a brief description of DGP, Method, Evaluator, and Visualizer might be helpful. Could even integrate this in Fig 1
  • "The custom functions are then and optionally parameterized, which are..." is a bit long and difficult to parse. Maybe break into a couple sentences. Also, I think the alpha example should be changed to sd.
  • Consider modifying: "The Experiment stores references to the first four objects along with the DGP and Method parameters that should be varied (specified in add_vary_across()) during the simulation run." A concrete example of what we mean by "varying across" parameters might be helpful. E.g., if we vary across sample size n (n = 100, 200, 300), then run_experiment(n_reps = 10) will run the ten reps for each DGP-sample size combination. [Though we need to think about this phrasiing carefully, since only DGPs with n as an argument will be varied]

Figure 1:

  • I think we can include some text or guides about how to read this diagram. Eg., in the caption, add something like "each box is a core R6 class underlying our grammar of simulations. small boxes are the inputs into these R6 classes and how they relate. Main methods for each class are provided at the bottom of each R6 box". It might be unclear to the reader what *_params, *_fun refers to since I don't think they are mentioned in the main text. It might be best to make this figure as self-contained as possible.
  • probably should take a screenshot without the spellcheck red squiggly lines...

@PhilBoileau
Copy link
Collaborator

PhilBoileau commented Apr 21, 2023

Very nice work, James! This draft is in great shape. I have a few minor suggestions and have made some slight changes to some wording in a few areas. Feel free to rollback the commit if you don't like them. My suggestions are listed below. Don't feel like you need to implement if they don't feel relevant.

  1. First sentence of Summary: You might consider adding a follow-up sentence with simulation study use cases. Here's a first attempt: "Examples include the comprehensive benchmarking of procedures developed to solve a common task, and the conducting of confirmatory experiments demonstrating that novel methods achieve their theoretical guarantees."
  2. Second paragraph of Summary: Some readers might be unfamiliar with "tidy" concepts, or might not realize that this approach to designing software extends beyond the tidyverse. They might might appreciate a reference to the tidyverse design guide.
  3. Code: I agree with all of Tiffany's suggestions. You might consider going one step further by defining (short) dgp, method, eval and viz functions. This'll make the exposition of simChef's functionality less abstract. It'll be obvious to readers that n is an argument common to dgp1() and dgp2(), for example. I'm happy to take a stab at this if you'd like.
  4. Definitions of the DGP, Method, Evaluator, and Visualizer objects: As Tiffany has already mentioned, a brief descriptions of these objects would be nice. Perhaps in a table that can be referenced in the text? A one-sentence summary for each class is probably enough.
  5. Case study: This isn't a comment, but let me know if you'd like me to make any changes to the case study repository so that it better aligns with this JOSS submission.

Again, great work! Let me know what I can do to help.

@jpdunc23
Copy link
Contributor Author

jpdunc23 commented Apr 28, 2023

2609c93 incorporates some of the above feedback. Outstanding items:

  • [ ] update example code to include simple dgp / method / evaluator / visualizer functions and add code comments
  • updates to Figure 1 to focus on experiment and correspond to code example (@tiffanymtang currently working on this, but let me know how/when I can help)
  • possibly add a Figure 2 with focus on DGP, Method, Evaluator, and Visualizer, and give definitions for each in the main text

Please let me know if I missed anything.

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

Successfully merging this pull request may close these issues.

3 participants