Skip to content

Draft vignette porting The Bank: Part II from SimPy#106

Merged
Enchufa2 merged 4 commits intor-simmer:masterfrom
nacnudus:bank2
Jul 5, 2017
Merged

Draft vignette porting The Bank: Part II from SimPy#106
Enchufa2 merged 4 commits intor-simmer:masterfrom
nacnudus:bank2

Conversation

@nacnudus
Copy link
Contributor

@nacnudus nacnudus commented Jul 4, 2017

I wasn't sure what to name the file, so it is simmer-08-bank-2.Rmd for now.

@Enchufa2
Copy link
Member

Enchufa2 commented Jul 4, 2017

Many thanks! I'm travelling to the useR!2017 conference today, so I'll review this PR in another moment.

For now, please rename the file as simmer-04-bank-2.Rmd.

@codecov
Copy link

codecov bot commented Jul 4, 2017

Codecov Report

Merging #106 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #106   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          22     22           
  Lines        2561   2561           
=====================================
  Hits         2561   2561

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72e7359...f97755e. Read the comment docs.

@nacnudus
Copy link
Contributor Author

nacnudus commented Jul 4, 2017

No hurry, enjoy useR! Best wishes for the Simmer presentation.

@Enchufa2
Copy link
Member

Enchufa2 commented Jul 4, 2017

The Travis build has errored because there are some namespace conflicts with dplyr. To avoid this, as you can see in the other vignettes, I prefer not to load dplyr, but to call its functions explicitly from its namespace, i.e., dplyr::select() instead of select(). So please remove library(dplyr) and prepend dplyr:: to every dplyr method.

To avoid a namespace conflict
@nacnudus
Copy link
Contributor Author

nacnudus commented Jul 4, 2017

Thanks for the tip, I was struggling to figure that one out.

@Enchufa2
Copy link
Member

Enchufa2 commented Jul 5, 2017

Some more comments:

  • There is a typo: "You man all enter".
  • In "Monitors", I would specify that the get_mon_*() functions return a data frame (that is growing during the simulation, so that, although possible, it is computationally expensive to call them from inside a trajectory), while the others return a numeric.
  • Please add simmer.plot to the list of requirements in the first chunk.
  • In "Plotting a Histogram of Monitor results", you talk about line numbers, but I can't quite understand where you are pointing to (maybe I read it too quickly?)
  • Please update the NEWS file with the following under the "Minor changes and fixes" section:

New "The Bank Tutorial: Part II" vignette, by Duncan Garmonsway @nacnudus (#106).

trajectory() %>%
timeout(openTime) %>%
log_("Ladies and Gentlemen! You man all enter.")
log_("Ladies and Gentlemen! You can all enter.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that was a "may". 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, you're right! Too late now ;-)
temp

@Enchufa2 Enchufa2 merged commit 6c070f2 into r-simmer:master Jul 5, 2017
@Enchufa2
Copy link
Member

Enchufa2 commented Jul 5, 2017

Many thanks, merged!

@nacnudus nacnudus deleted the bank2 branch July 5, 2017 21:44
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.

2 participants