Skip to content

Peer review from Fatemeh (start -> model building) #134

@amyheather

Description

@amyheather

Feedback from @Falidoost - thank you!

General:

I went on with the book and codes provided, and run the codes in my own laptop. I used my student/ educator lens for providing the comments :) Please feel free to ignore any of comments if you feel unnecessary.

  • I very liked the easy to understand language of the book and the "Test yourself" section! The latter made the book interactive. Just as a suggestion, having the answers/solutions for the tests, would be helpful for improving learning experience. There is an educational application for ISO called "Mimo", which uses the coding tests for learning purposes, it might be worth to take a look for some more test yourself examples. I know that the book is not meant to be a coding tutorial, but some people (including me) might be not professional coders, and these test might be helpful for them.

  • I think readers usually follow along with the codes provided, so they won't be challenged for the Test yourself sections like the one provided in the following image:

--> Thank you for both these comments re: test yourself. I need to have a think about what best to do... I've not addressed yet, but have made a separate GitHub issue to focus on this (#145), so have ticked these off here, but still working on this in the new GitHub issue.

Image
  • From chapter 2 there is a "choose your language" icon in the book, which was nice, however it would be helpful if you could have mention to this option somewhere in the book. --> Now described on the introduction page for the step-by-step guide
Image

I liked the introduction and setup section for Github! It was very easy to follow! --> Thank you!

Ch1 - Introduction


Discrete event simulation (DES)

  • In the table below, I think it should be entities AND "resources". --> I've tightened up the wording to hopefully make it clearer here - that queues form when entities need a resource that is currently busy and so have to wait for the resource to become available
Image

Reproducibility and RAPs:

  • Typo in the following section:
Image
  • Is the guidance/framework of RAP mentioned in the following image different based on industries? --> I've add explanation of why the frameworks were chosen + their relevance, on the frameworks pages as well as on the reproducibility page briefly too.
Image

Ch2 - Setup


Version control

  • In section related to create a branch -- It would be helpful to add a screenshot of terminal in VS as it was challenging for me when I was working with VS first time to find where the terminal is. We already saw a screenshot of Git Bash, it would be also good to familirise readers with "terminal" in the VS, if possible. --> Have add GIF + explanations on this in section where clone repository

  • This might be out of scope, but I think it would be helpful to give a hint/ short Youtube video on how to switch between repositories or add repositories in the working directory. --> Have add suggestion to use separate windows

  • Please feel free to ignore this comment, it is just a reflection from a student perspective: While I very liked the explanations about the git and repositories, I think discussions about how the git could be managed or shared might be a little exccessive information, however as you already mentioned this is important for projects with multiple team members. If the audiences of the book are targeted at students/ ECRs they might be less exposed to a modelling where multiple people are involved in coding stage. --> Have add caveat at start of GitHub organisations section to say that is mainly for team projects than for solo workers

Environment

  • Before create environment file, would it be possible to have a short section on setting the working directory? --> Have add before the conda commands

Structuring as a package

  • In section Option A and Option B: Would it be possible to replace envname with the des-rap-book? It would improve the readability, because in the environment hyperlinked in this section the name refers to des-rap-book and continuing the example provided by the book with the same namings will avoid confusion. I provided a snapshots of the sections I refer to: --> Have modified environments + packages page... there is still minimal environment example, but page now clearly says to delete that and move onto full example with new name. Kept minimal as good for explaining basic concepts, but you're right that it wasn't very clear what environment people should have.
Image Image
  • The dependecies provided in the book in sections e.g. Environments > Test youself, is different from the one in the Structuring as a package in the section related to opetion B as shown below in the snapshot. I think it would be helpful to consistently using same naming, and concepts for avoiding confusion. --> Have made the package page use des-rap-book, and moved the des-rap-book from "test yourself" to within main environments page under new section "environment for book".
Image Image
  • I noticed something when I was searching for OOP through the book using search tab of the book. The third item appeared in in the search does not exist when I click on it, please see the following snapshot: --> This is because it is on the R version of the page. You are right, this is quite confusing! I can't figure anyway to alter the search results, so have removed search now.
Image
  • In the Code organisation > Test yourself; would you mind providing hint/ solutions?

Ch3 - Model Input:


Input modelling:

  • I had problem for installing and importing distfit package. I faced the error "No module named 'matplotlib.backends.registry'" and tried to fix it using VS fix option, not fixed at the end but the codes run successfully! I am not sure where the problem is! --> Afraid I'm also not sure as haven't been able to reproduce this. I have created a separate issue dedicated to this (Environment bug identified by Fatemeh #142) and will monitor if anyone else encounters the same issue - but for now, cannot identify any further actions to take.

  • In the following section, can we say we observe no trends, etc. after the plots? I mean providing some interpretations after showing the time series plots. --> Added interpretations.

Image
  • Also providing an example or refering to an example with seasonality might be helpful for guiding the reader/ educator on what they should look for regarding seasonality/ trends. --> Described what these would look like.

  • The previous comment, about providing some interpretations, also applies for Histogram part. --> Added interpretations.

Image
  • Plots in step 2 are not completely shown, look at the following screenshot: --> Fixed.
Image
  • Just a suggestion: It might be helpful to have a different dataset in "Test yourself", to be practiced with solutions provided. It would have been helpful to map and test understanding in another different example --> Addressing in new test yourself GitHub issue as mentioned above

Parameters from script:

  • Space required:
Image
  • Seeing "not applicable" in the second example felt a bit confusing at the beginning, though then I understood what you meant :) --> Have tried to re-write a little clearer.
Image

Parameters from file:

  • I could not find anywhere that print_dict is defined. I assume that this is a function for printing the parameters with intendation, though I think it would be good to mentioned somewhere about it. --> For simplicity, have removed print_dict() and switched to just using json.dumps() (which is explained in code when first used on page).

  • I am not sure if there were any reference for creating parameters_file_resources folder in these sections so far. Thus, I needed to either build one or remove it from the following code (in "Create data dictionary"): --> Have changed so instructs to create inputs/ folder.

Image

Ch4 - Model building:


Entity generation:

  • In the first para, in section Creating a simple simulation model with arrivals, when you are mentioning to a conceptual model, it might be helpful if you could have one for the simple simulation model, e.g. as a flowchart, to refer to. --> Add an intro section describing the conceptual model used in book with flowchart, as well as linking out to a good resource on how to do conceptual modelling (as we have not covered that).

  • In "Distribution registry" when I move the curser to the code area, some of the lines including those shown in the image below, become bold. I think the feature is to show that these lines are added to the previous code, I found this very helpful :)

Image

Entity processing:

  • following on my previous comment on conceptual model, it would be good to complement the previous conceptual model in this section with the added process and resource (a consultation process with a doctor).

Logging:

  • Can we add a log for "Patient 1 ends consultation". This might be helpful for understanding the sequences in the model and a way for verification as mentioned in the book earlier. It could also be added in the "Test yourself"

  • I am wondering if the logs can be saved in a csv file, it was a bit handier for me but feel free to ignore this if you feel it is unnecessary ** --> At the moment the logger is intentionally designed as a debug/verification tool rather than a structured data output, which is why the examples write to the console or a .log file rather than a CSV for analysis - but thank you for the suggestion!**

Metadata

Metadata

Assignees

Labels

peer reviewFeedback from peer review of the repo

Type

No type

Projects

Status

Done

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions