CodeReview Round 1

[monicadragan]

Hi Ankur,

Here are my comments after reading your code (pacf implementation):

Your code is well organized (small functions, you make use of methods in Enumerable) and easy to read. Also, you provide useful comments, with references.

Some suggestions:

• type error handling for function arguments (should be useful, as long as you're using a lot of predefined types from other modules)
• avoid return where not required (to be fair, I used returns myself too, but I think this is not ruby stile and also my reviewer commented on this)
• line 62: "ts = series". Not sure what you want to do here, but if you want to avoid mutation of the array you should clone the variable (i.e "ts = series.clone")
• line 73: r = Array.new(k + 1) { 0.0 } -> should be more elegant like this: "Array.new(k + 1, 0.0)"
• line 76: avoid for loops; you can use (1..k).each do |i| ... (you are avoiding this in other parts of the code, though)
• be consequent with using (or using not) parenthesis for the arguments of the functions

Hope this was useful. Btw, I was learning a lot reading your code! You have an interesting and useful project, so very good luck!

Monica

[AnkurGel]

@monicadragan

I am extremely grateful to you for taking out time to review the code. I loved your suggestions and found them very useful.

• I didn't include error-handling yet since I am expecting to include more code in PACF. I will intimate you as soon as I add that code with appropriate error handling. :+1:
• Yes, I should have avoided return statement; though personally I find it more readable to include in a complex method where the variable to be returned is being modified in internal code and can in-turn be returned somewhere else in between.
• ts = series. Yes, that has to be removed. Thanks for brining that in notice :)
• Exactly, Array.new(k + 1, 0.0) is definitely the conventional way. :) I did use this convention in line 89.
• Avoid loops. Definitely. I will refactor that with upto().

Again, I am greatly pleased and very grateful for the review. I will commit with all suggested changes by you, @mohawkjohn and @clbustos shortly and reflect here.

Thanks everyone :)

[agarie]

Hey @AnkurGel, I'm centralizing SciRuby's gems in the organization repositories. Can you reopen your PR on sciruby/statsample?

I imagine that some of these commits went into statsample-{timeseries,glm}, but if there's something that could be useful to statsample "core", please open a new PR there.

Thanks! :v:

[AnkurGel]

@agarie Yes. Most went in independent gems -timeseriees and glm. I will have a look at what went independently in statsample and give a PR. :smile: