hw09 ready for grading

[abishekarun]

SHA: f00250e1fde3564679f77632eae373a72de513b5 Readme file

Package

*functions fact_fun() and nthFibonacci modified after suggestions.

[jardnzm]

Hi @abishekarun ,

• ReadMe file reflected what you learned and struggled with. Also contained the related link to some useful page. check.

• Installation worked fine.

• package?mathfunctions page included, check.

• After install, I counted that this package includes like 11 exported functions, which is quite functional, viggnetts was informative to describe the operation, inputs and outputs of the functions.

• contains three different log inside one log script, and that clean a bit the directory for sure.

• There is an warning message in naturalLog if I set x as a vector Warning message: In if (x <= 0) stop("x must be >= 0") : the condition has length > 1 and only the first element will be used

• I notice in your code you have if(!is.numeric(x))stop("x must be numeric") to check input class, and I just wanna share another way to do that assertive::assert_is_numeric(x)

Overall a solid homework. You have completed all the required tasks, and it seems like you understand the mechanics behind building a package, which is important.

Best, Ziming

[mylinhthibodeau]

Dear @abishekarun,

Good job on homework 9. I was able to install you package successfully, but there were a few problems with the functions of your package.

Homework requirements:

• Define and export at least one new function: You have written scripts for several additional functions in your package.
  • nthroot()
  • naturalLog()
  • binaryLog()
  • commonLog()
  • nthFibonacci()
  • check.integer()
  • logarithm() (available internally only)
  • fact_func()
  • sum_series()
• Give function arguments sensible defaults: yes (e.g. default plot_it=FALSE for functions, and n=2 for nthroot()) for all functions. The nthFibonacci() function has a default first term of 0 and second term of 1, which is in keeping with the classical Fibonacci serie, but I am wondering about validity of asking the user to input terms of the Fibonacci series (see function-specific comments below).
• Use assertions to add some validity checks of function input: some functions do have validity check, but the code for these checks is not working in some instances (see function-specific comments below).
• You have general documentation accessible with help(mathfunctions) and a general package vignette accessible (using_mathfunctions). You have individual vignettes for each function. You also added a link to the base R log vignette page.
• Document all exported functions: yes, functions are documented.
• Include at least three unit tests for every function that is exported. There should be at least one expectation for success and one for failure: in the tests/testthat folder, scripts for testing functions are available: test_naturalLog.R, test_nthFibonacci.R, test_nthRoot.R, test_square.R, test_sum_series.R.
• Your package should pass check() without errors (warnings and notes are OK, though it would be great if there were none):
  • I could not check if the testing went smoothly, but I suspect there might have been some error messages for some of the functions? (see function-specific comments below). One consideration would be to use sum(seq()) to test the sum_series() function.
  • I didn't think of adding my reviewers to my package repository before today, but I have added the stat545/547 team now so that they can clone my repository and run check(). I think it could be a consideration as a requirement for future packages so that cloning the repository is possible for review and running check() perhaps?
• Keep updating the README and vignette to show usage of all the functions in the package: the README is succinct, but there is a risk of confusion as it mentions the log() function (which is instead function logarithm() in the package) and nthRoot() function (which is instead nthroot() in this package).
• Push your package to GitHub, preferably a public repo: yes, this has been completed !
• Write your reflections in a document: yes, this is available in the main homework repository through a readme file here. Specific examples of challenges could be helpful in helping troubleshoot some issues in the package.

General comments:

• I noted that your nthroot() function and your R script (nthRoot.R) has a small difference (capital vs lower case letter "R"), one idea would be to consider always using lower cases as a standard as it might be safer to avoid confusion in the future.
• Your functions (e.g. logarithm(), new_fact() have a different R script name (log.R, factorial.R), and I am not sure about this, but I thought it was a convention to have the same names for the R scripts and the associated functions?
• I would recommend that each function has it's own script at the beginning, because this makes it easier to troubleshoot and identify the cause of the problem when issues arise.

Function-specific comments

I have written some function-specific feedback and ideas below. Most of the material below is not meant as a peer review, but instead meant to help you troubleshoot some issues in your code.

• nthroot() - I like that you figured out this way cleaver line res <- sign(x)*abs(x)^(1/n) to deal with the presence of positive and negative numbers in a vector. However, your if statement to check for the presence of negative numbers and an even root number does not work.

  nthroot(1:10,2,plot_it=TRUE)
  [1] 1.000000 1.414214 1.732051 2.000000 2.236068 2.449490 2.645751 2.828427 3.000000
  [10] 3.162278
  Warning message:
  In if (!(n%%2 == 1 | x >= 0)) stop("x must be >= 0 or n must be odd") :
  the condition has length > 1 and only the first element will be used

  I have provided an example of your code that would accomplish this below (I use positive if statements, I find them easier to follow).

  nthroot <- function(x,n=2,plot_it=FALSE){
  if(!is.numeric(x))stop("x must be numeric")
  else if(!is.numeric(n))stop("n must be numeric")
  
  # else if statement below edited by My Linh Thibodeau (MLT)
  else if(
      (n %% 2 == 0)   # MLT: If n is an even number, n modulo 2 will be equal to 0
      &&              # MLT: AND statement (both conditions needs to be present to return error)
          (x < 0))      # MLT: if x is a negative number 
      stop("If x is negative, n must be an odd number.") # MLT: if x is negative AND n is not odd, then an informative error message is returned.
  
  res <- sign(x)*abs(x)^(1/n)
  
  if (plot_it) print(ggplot2::qplot(x, res))
  return(res)
  }

• naturalLog() - See comment below on logarithm() function.
• binaryLog() - See comment below on logarithm() function. The term "binary log" is used to record the changes to a database (e.g. as explained by MariaDB), so the name of the function has a risk of confusion, a consideration would be using something like "base2_log()" instead.
• commonLog() - See comment below on logarithm() function.

• logarithm() - This function can not be found in the package, but instead it is being used internally by the other functions. However, it also has a little error in the logarithm() function because the functions depending on it return the error messages even when adequate input is provided.

  > naturalLog(seq(1,5), plot_it = TRUE)
  [1] 0.0000000 0.6931472 1.0986123 1.3862944 1.6094379
  Warning message:
  In if (x <= 0) stop("x must be >= 0") :
  the condition has length > 1 and only the first element will be used

  For the logarithm function, you could try to breakdown each condition individually into more detailed statements, for example:

  # Code below edited by My Linh Thibodeau
  logarithm <- function(x,n=exp(1),plot_it=FALSE){
  if(any(x<=0)) {                                                     # check for any negative numbers in x 
      stop("All elements of x must be >= 0")} # If a negative number is present in x, stop and return an informative error message
  
  else if(!n>=1) {                                                    # check for n is equal or greater than 1 - if condition not respected, then stop
      stop("n must be >= 1")}                                 # return informative message if n is not equal or greater than 1
  
  else {                                                                      # else (if conditions above respected) proceed to the function
      res <- log(x, base=n)
  }
  
  if (plot_it) print(ggplot2::qplot(x, res))
  return(res)
  }

• nthFibonacci() - Unfortunately, this function is not coherent. According to Fibonacci sequence resources on Wikipedia and math is fun and this code example here, your code does not comply to the general principles of Fibonacci sequence (starts at 0 or 1, etc). For example nthFibonacci(1) returns 3 in your function and I believe it should return 0 or 1 perhaps? I have provided an example of a potential classical nthFibonacci() (initial term 1) and returning a vector starting at the nth number (first_term_indice) of Fibonacci sequence and providing the number of terms specified by the user (number_terms):

  nthFibonacci <- function(first_term_indice = 3, number_terms=5){
  if(first_term_indice<1) 
      stop("first_term_indice must be >= 1")
  
  else if (number_terms <1) 
      {stop("number_terms must be >=1")}
  
  else if (first_term_indice==0 || number_terms == 0)
      {stop("number_terms and first_term_indice must be >0")}
  
  else if(!(sum(abs(round(first_term_indice)))-sum(abs(first_term_indice)) == 0))
      {stop("first_term_indice must be a positive integer")} # check if first_term_indice is positive
  
  else if(!(sum(abs(round(first_term_indice)))-sum(abs(first_term_indice)) == 0)) 
      {stop("number_terms must be a positive integer")} # check if number_terms is positive
  
  else {      
      fibo_vector = vector(mode = "numeric", number_terms)          # Create an empty numeric vector of length fibo_max
      fibo_vector[1] <- 1             # First element of Fibonacci sequence is 1 
      fibo_vector[2] <- 1             # Second element of Fibonacci sequence is 1 
      for (i in 3:(first_term_indice+number_terms)) {         # Starting at 3, each new element added is the sum of the two preceding elements in the fibo_list
          fibo_vector[i] <- fibo_vector[i-1] + fibo_vector[i-2]
      }
  
  }
  res <- fibo_vector[first_term_indice:(first_term_indice+number_terms-1)]        # Returns the numbers between the 
  if (length(res !=0)) {       # if the length of vector res is not zero, return result
      return(res)
  }
  }

  For example, asking for 7 terms in total, including and starting at the 5th term of the Fibonacci sequence.

  > nthFibonacci(5, 7)
  [1]  5  8 13 21 34 55 89

• check.integer() - This function works with one number, but not a list/vector. When I proceed with check.integer(c(1, 4, 5.5)), it returns [1] FALSE FALSE FALSE. Also, I noted that you call the input "x" or "n", a suggestion would be to use the same variable (perhaps always "n" in your case?). The purpose of function code line !grepl("[^[:digit:]]", format(x, digits = 20, scientific = FALSE)) was unclear to me.
• fact_func() - This function did not return any result for numeric input (fact_func(5) returned no result). In order to make a reflective function, my understanding is that you need to return the result to the function, so for example, your code would work with:

  fact_func <- function(n){
  if(n<0) stop("n must be >= 0")
  else if(!check.integer(n)) stop("n must be positive integer > 0")
  res<-1
  if(n==0)
      return(res)
  else
      return(n*fact_func(n-1)) # Modified from original code by My Linh Thibodeau 
  }

• sum_series() - The function and the vignette are not very clear to me. I understood that you were trying to code the equivalent of sum(seq(12, 51, 2)) for your sum_series(12,2,20), but I would recommend adding further details to the vignette to clarify the arguments required by the function.

Overall, writing a package is a substantial challenge and I think that you did well in the light of the difficulty of this specific task !!

Regards, My Linh Thibodeau

[abishekarun]

Dear @mylinhthibodeau , First of all, thank you for such an elaborate review for my homework 9. I would like to clarify few things with respect to my homework.

1) I wanted to implement more functions. So, I didnt have time to implement the functions in such a way that they accept list/vector inputs. As of now, they are developed such that only numbers as input works and it doesnt work for lists/vectors.I tested them and debugged them with only numbers and not vectors.This is the reason I believe you were getting the errors for some of the functions such as nthroot.I will try to incorporate this functionality soon in order to avoid these errors.

2) I understand that fibonacci series has first term has 0 and second term has 1. I wanted to develop a custom function that follows fibonacci series by default but in general follows that pattern according to user's input. I believe the name of the function should have been different to avoid the confusion.

3) I thank you for pointing out the error in the readme file regarding the function names. I have corrected the same. Also, I had to rename the function logarithm since there was already inbuilt log function. I understand the confusion with function name and file name, will try to keep it consistent from next time. Thanks for the suggestions.

4) The nthFibonacci function that I developed was intended to return the nth number excluding the first and second term. I have checked the function and there was a small typo. I have corrected the same and modified it to return the numbers including the first and the second term now. Thank you so much for pointing it out.

5) The fact_fun() similarly had a typo as I was trying to return a variable when n is equal to zero. I have fixed it now. Thanks to pointing it out and your suggestion.

6) I used the !grepl("[^[:digit:]]", format(x, digits = 20, scientific = FALSE)) to check if the number is an integer or not as I couldn't find a way to filter out the integers alone using the inbuilt functions.

7) Finally, I tried developing sum_series() in order to get the sum of an arithmetic progression (AP) series. I am sorry that I didn't mention this clearly in the documentation. I have changed that upon your suggestion as well.

Also, I think it is great idea to add the stat545/547 team now so that they can clone my repository and run check() for testing the package. I have gone ahead and followed your suggestions and added them now. Thanks for that.

Thank you so much for such a detailed interview as it helped me correct the mistakes and improve the functionality. I have corrected some of the mistakes. I will try to incorporate the vector/lists support soon. Thank you once again for your elaborate review and valuable suggestions.

Regards abishekarun