mulias / roc-array2d

Fixed size 2D arrays for Roc
ISC License
5 stars 0 forks source link

flipY is the same as flipX #2

Closed ryanb closed 9 months ago

ryanb commented 9 months ago

They appear to have the same implementation.

flipX : Array2D a -> Array2D a
flipX = \@Array2D array ->
    startAcc = @Array2D array
    List.walkWithIndex array.data startAcc \acc, elem, listIndex ->
        newIndex =
            listIndex
            |> arrayIndexOf array.shape
            |> flipIndex array.shape X

        set acc newIndex elem

flipY : Array2D a -> Array2D a
flipY = \@Array2D array ->
    startAcc = @Array2D array
    List.walkWithIndex array.data startAcc \acc, elem, listIndex ->
        newIndex =
            listIndex
            |> arrayIndexOf array.shape
            |> flipIndex array.shape X

        set acc newIndex elem
mulias commented 9 months ago

Oops, definitely something I meant to get back to. I think I wasn't quite sure how to communicate what these functions did, or if they should maybe be flipRows and flipCols. Still not sure in general about x/y vs rows/cols, but I think for now we can stick with these names. Does this fix look good to you? https://github.com/mulias/roc-array2d/pull/4

ryanb commented 9 months ago

@mulias PR looks good to me.

I'm not sure about naming. I personally like x/y because it's short, communicates well and fits the "2D" name.

That said, flip could be taken either way since it's not very clear which way it's flipping based on the name. Perhaps look at other libs to see how they name this.

mulias commented 9 months ago

I agree about names, but I think that's part of a larger design question. At this point I'm not sure if it's worth it to generalize the library to include 1D, 3D, or ND data structures, and if we did that how it would impact function names.

I merged the fix pr so I'll close this for now.