Anirban166 / testComplexity

Asymptotic complexity testing framework
https://anirban166.github.io/testComplexity/
Other
36 stars 2 forks source link

confusing example, rename data.sizes?? #46

Closed tdhock closed 4 years ago

tdhock commented 4 years ago

Hi @Anirban166 I read your doc blog about expect_x_time and that is helpful.

  expect_loglinear_time(PeakSegPDPA(rpois(data.sizes, 1), rep(1, length(rpois(data.sizes, 1))), 3L), data.sizes = 10^seq(1, 4, by = 0.5))

what is data.sizes in this example? It appears three times so I think this is potentially confusing. In the first argument data.sizes is ONE of the values that are specified by the named data.sizes= argument, right? Because it is plural (sizes) I would think it is a vector of more than one number. To avoid that confusion can you change it to N instead, as we discussed?

  expect_loglinear_time(PeakSegPDPA(rpois(N, 1), data.sizes = 10^seq(1, 4, by = 0.5))

Also to clarify the example can you please use a variable name as argument to PeakSegPDPA, and omit the second argument? e.g.

expect_loglinear_time({
  data.vec <- rpois(N, 1)
  PeakSegPDPA(data.vec)
}, data.sizes....)

does that make sense to you?

Anirban166 commented 4 years ago

Hi @Anirban166 I read your doc blog about expect_x_time and that is helpful.

  expect_loglinear_time(PeakSegPDPA(rpois(data.sizes, 1), rep(1, length(rpois(data.sizes, 1))), 3L), data.sizes = 10^seq(1, 4, by = 0.5))

what is data.sizes in this example? It appears three times so I think this is potentially confusing. In the first argument data.sizes is ONE of the values that are specified by the named data.sizes= argument, right? Because it is plural (sizes) I would think it is a vector of more than one number. To avoid that confusion can you change it to N instead, as we discussed?

  expect_loglinear_time(PeakSegPDPA(rpois(N, 1), data.sizes = 10^seq(1, 4, by = 0.5))

Yes your right, N would look less confusing than the repeated usage of data.sizes in the expression. There are two simple ways I can go about this in asymptoticTimings based on the arguments/parameters, so I would like to know which one is better:

Also to clarify the example can you please use a variable name as argument to PeakSegPDPA, and omit the second argument? e.g.

expect_loglinear_time({
  data.vec <- rpois(N, 1)
  PeakSegPDPA(data.vec)
}, data.sizes....)

does that make sense to you?

Yep, perfectly does! but in that code above we would require to give a value to max.segments atleast right (and not just include data.vec)? Because otherwise it would throw an error for max.segments (like its not an integer), since in your code the default is set to NULL

works fine:
image

will make the changes to code & blogs but please check if anything else needs to be improved as well, also I guess I should make the same changes for the cDPA test case as well? (and other functions as well)