charleskawczynski / PlayingCards.jl

A package for representing playing cards for card games.
MIT License
14 stars 4 forks source link

Add RNG to Shuffle, Change Deck to AbstractVector #27

Closed SBuercklin closed 1 year ago

SBuercklin commented 2 years ago

This PR makes two main changes:

shuffle! (and now shuffle as well) can be passed an AbstractRNG to let the user define their own RNG. If no RNG is provided, the default RNG is used instead. I also rewrote shuffle! to use the in-place shuffle!(::Vector) method so that shuffling the deck is non-allocating.

Along with that, I wrote an out-of-place shuffle method which allows you to retain your original deck.

I also changed the type parameterization for Deck{<:Vector} to Deck{<:AbstractVector{Card}. This forces the contents of Deck to be a vector of Cards, where before the Deck could contain any Vector. Using an AbstractVector also lets users use an alternative vector type, like an SVector to stack-allocate the Deck.

I'm happy to throw out the Deck{<:Vector} change; I only included it because I didn't feel like a second PR was necessary. The shuffle RNG changes are my primary interest.

I also wanted to say thanks for putting this package together. It's a very space-efficient implementation of a standard 52-card deck, and I'm looking forward to playing around with it :)

charleskawczynski commented 2 years ago

Thanks, @SBuercklin, for the changes, they look great! I'm happy to merge, but could you squash the commits first?

charleskawczynski commented 2 years ago

It's a very space-efficient implementation of a standard 52-card deck, and I'm looking forward to playing around with it :)

It is! My original implementation was very space inefficient, and then I borrowed basically most of the card representation design from Stephan Karpinski's (unregistered) Cards.jl, so credit to him for the great design 🙂

SBuercklin commented 2 years ago

FWIW, the Allocations test is failing for me locally on 1.7.3 but passing on LTS 1.6.6. I get 352 instead of 304 for alloc.

I haven't looked too deeply into why that might be happening yet, but if you have any ideas it could be worth opening an issue upstream. If I get a chance tomorrow I'll ask about it on the Slack.

charleskawczynski commented 2 years ago

FWIW, the Allocations test is failing for me locally on 1.7.3 but passing on LTS 1.6.6. I get 352 instead of 304 for alloc.

Same here, I assume it's just a small upstream regression. I'm not too worried about it, I think I was just trying to be extra careful about allocations while I was trying to optimize some downstream packages. I'm monkey patching the test in #28 for now.

codecov[bot] commented 2 years ago

Codecov Report

Merging #27 (36e7d17) into main (b42ea56) will decrease coverage by 4.50%. The diff coverage is 42.85%.

@@            Coverage Diff             @@
##             main      #27      +/-   ##
==========================================
- Coverage   98.80%   94.31%   -4.50%     
==========================================
  Files           2        2              
  Lines          84       88       +4     
==========================================
  Hits           83       83              
- Misses          1        5       +4     
Impacted Files Coverage Δ
src/PlayingCards.jl 94.20% <42.85%> (-5.80%) :arrow_down:
charleskawczynski commented 1 year ago

Not sure why I didn't merge that earlier!