Closed ErikSchierboom closed 2 months ago
That made me think for a while!
It seems that the intersect()
on line 6 isn't doing quite what we expect. On debugging, neighbor_coords
is an empty list, which is obviously wrong (or at least unintended).
Now that we have tidyverse
in the test runner, it is possible to add library(dplyr)
at the top of the file, then replace intersect
with inner_join
. All tests passed when I tried this locally.
Thanks to stackoverflow for this fix.
That still leaves the usual line-length linting problems, of course.
I suspect @jonmcalder would have a better solution, if he is around.
Also, using a matrix looks fine to me. Maybe my judgement is warped by long experience with Matlab and NumPy, but I don't see why it shouldn't also be idiomatic R. This data type has been in the base language for a long time.
Thanks @colinleach! I'll wait for @jonmcalder to see if he agrees too, and then we I'll continue fixing things.
Yes I agree that it makes sense to use a matrix implementation for this.
Colin is correct that you can't use intersect()
in that context because it's meant to work on vectors (and expand.grid()
produces a data frame). Using either dplyr::intersect()
or dplyr::inner_join()
instead would work here.
I don't have a different approach to suggest right now (and can't give more time to this) but I think there are some interesting approaches here if you want to explore a little more.
The implementation and tests are the main thing and in my view the example solution only really needs to demonstrate a possible way to solve the exercise (ideally as elegantly and idiomatically as possible but that can be subjective). So I'm sure I'll be happy with whatever you guys decide here as long as you both are happy.
Minor suggestion: maybe the linting should be moved to its own, separate GHA workflow. That will allow the contributor to verify that the tests are passing without having to satisfy the linter.
ideally as elegantly and idiomatically as possible but that can be subjective
I'll happily let others submit a PR to improve upon my solution :)
Minor suggestion: maybe the linting should be moved to its own, separate GHA workflow. That will allow the contributor to verify that the tests are passing without having to satisfy the linter.
Yes probably a good suggestion. Out of interest did you consider using {styler} and/or {precommit} to help avoid linting issues as outlined in the (now very outdated I'm sure) ReadMe?
I hadn't done that. Probably should have. Sorry
@jonmcalder I've fixed CI
@jonmcalder I've fixed CI
Yup that's fine. Thus far we've managed to get by with only base R (i.e. no tidyverse) for all example solutions so haven't needed it and it made sense to keep CI as lean as possible. But it's quite likely we'll rely on {dplyr} for one or more example solutions going forward so probably makes sense to include it.
no offence of course @ErikSchierboom
None taken! There must be someone that can do this a ton better.
I'm merging this to have it be available, and then the updated example solution can be PR'ed later.
I hadn't previously thought about the impact on CI, after adding tidyverse
to the test runner. I suspect that at some point we will want more than just dplyr
available. I'm thinking particularly of stringr
: a pretty major improvement on string handling in base R.
I understand the bias towards keeping CI lean, but what are the practical implications of adding more packages? Slower? More expensive?
I think this should work, but I can't seem to get the tests passing.
Also, let me know if using a matrix is a good idea here, or if I should use something else