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

Add game-of-life exercise #347

Merged
merged 4 commits into from
Sep 18, 2024
Merged

Add game-of-life exercise #347

merged 4 commits into from
Sep 18, 2024

Conversation

ErikSchierboom
Copy link
Member

@ErikSchierboom ErikSchierboom commented Sep 16, 2024

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

@colinleach
Copy link
Contributor

colinleach commented Sep 16, 2024

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.

@colinleach
Copy link
Contributor

I suspect @jonmcalder would have a better solution, if he is around.

@colinleach
Copy link
Contributor

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.

@ErikSchierboom
Copy link
Member Author

Thanks @colinleach! I'll wait for @jonmcalder to see if he agrees too, and then we I'll continue fixing things.

@jonmcalder
Copy link
Member

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.

@ErikSchierboom
Copy link
Member Author

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.

@ErikSchierboom
Copy link
Member Author

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 :)

@jonmcalder
Copy link
Member

jonmcalder commented Sep 17, 2024

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?

@ErikSchierboom
Copy link
Member Author

I hadn't done that. Probably should have. Sorry

@ErikSchierboom
Copy link
Member Author

@jonmcalder I've fixed CI

@jonmcalder
Copy link
Member

@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.

Copy link
Member

@jonmcalder jonmcalder left a comment

Choose a reason for hiding this comment

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

Cool - I'm happy with this implementation.

Still hoping someone will put up their hand to improve on Erik's example solution though - not sure if it's the nested functions or the mixing of operations on matrices and data frames with apply / expand.grid / dplyr::inner_join but it's not easy to read and seems like a bit of a code smell to me.

(no offence of course @ErikSchierboom - you're a polyglot so you're bound to approach things differently and code can be subjective - thanks for contributing this exercise!)

@ErikSchierboom
Copy link
Member Author

no offence of course @ErikSchierboom

None taken! There must be someone that can do this a ton better.

@ErikSchierboom ErikSchierboom merged commit 36d408c into main Sep 18, 2024
3 checks passed
@ErikSchierboom ErikSchierboom deleted the game-of-life branch September 18, 2024 10:31
@ErikSchierboom
Copy link
Member Author

I'm merging this to have it be available, and then the updated example solution can be PR'ed later.

@colinleach
Copy link
Contributor

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?

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.

3 participants