Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for array indexing #31

Merged
merged 5 commits into from
Mar 4, 2016
Merged

Fixes for array indexing #31

merged 5 commits into from
Mar 4, 2016

Conversation

karldw
Copy link
Contributor

@karldw karldw commented Mar 2, 2016

karldw added 3 commits March 1, 2016 20:33
As simple as devtools::use_testthat().
- Added tests for array and vector slicing.
- Added get.limit.index helper function to repr_matrix_df.r.  Given a vector length and a
desired limit, get.limit.index will return the indices for the limited vector.
- Fixed minor bugs in row numbers and in using elipses as factors.
@flying-sheep
Copy link
Member

oh wow, you’re awesome! 😊

stopifnot(obj_dim > limit) # otherwise this function should not have been run
left_or_top <- seq_len(ceiling(limit / 2))
right_or_bottom <- seq.int(obj_dim - floor(limit / 2) + 1L, obj_dim)
return(list(begin=left_or_top, end=right_or_bottom))
Copy link
Member

Choose a reason for hiding this comment

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

code style: spaces around =, and no returns at the end of functions (R returns the last statement automatically)

@jankatins
Copy link
Contributor

oh wow, you’re awesome!

+1

@takluyver
Copy link
Member

Woo, tests! 🎉

@karldw
Copy link
Contributor Author

karldw commented Mar 2, 2016

Thanks for the comments, will address them soon!
I also noticed repr_latex(iris) raises a warning due to factors. I'll fix that soon too. (I was loading the wrong version of repr.)

flying-sheep added a commit that referenced this pull request Mar 4, 2016
Fixes for array indexing
@flying-sheep flying-sheep merged commit cf25f53 into IRkernel:master Mar 4, 2016
@flying-sheep
Copy link
Member

👍 super cool, i’ll add more tests from here 😄

@@ -50,7 +60,7 @@ ellip.limit.arr <- function(
# data.tables can't be indexed by column number, unless you provide the
# with=FALSE parameter. To avoid the hassle, just convert to a normal table.
if (inherits(a, 'data.table'))
a <- as.data.frame(x)
Copy link
Contributor

Choose a reason for hiding this comment

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

This was a bug? Otherwise, this looks strange to change that without a change which introduces a above...

Copy link
Member

Choose a reason for hiding this comment

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

seems so. can’t find x in the code above 😃

@jankatins
Copy link
Contributor

Thanks for introducing the test framework: what a nice cosy feeling when a codebase gets tested automatically :-)

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.

4 participants