Do you prefer a lookup table or an algorithm? + duplicate code

dglaude commented 3 years ago

I want to write the code for Pimoroni 5x5 LED Matrix. The table and algorithm is very similar is for led_shim. Except that led_shim has 28 entries and 5x5 has 25 entries.

Q1: What is prefered way of writing the mapping

(see below for code extract)

Q2: How do I convince pylint that this duplicate code is OK? Or should I put the table/algo in a separate file and import? (it is very similar but maybe with 3 entries less, but having 3 extra entries is not hurting much.

Lookup used in keybow2040:

    @staticmethod
    def pixel_addr(x, y):

        lookup = [
            (120, 88, 104),  # 0, 0
            (136, 40, 72),  # 1, 0
            (112, 80, 96),  # 2, 0
            (128, 32, 64),  # 3, 0
            (121, 89, 105),  # 0, 1
            (137, 41, 73),  # 1, 1
            (113, 81, 97),  # 2, 1
            (129, 33, 65),  # 3, 1
            (122, 90, 106),  # 0, 2
            (138, 25, 74),  # 1, 2
            (114, 82, 98),  # 2, 2
            (130, 17, 66),  # 3, 2
            (123, 91, 107),  # 0, 3
            (139, 26, 75),  # 1, 3
            (115, 83, 99),  # 2, 3
            (131, 18, 67),  # 3, 3
        ]

        return lookup[x][y]

Algorithm used in led_shim (not exactly the same table):

    @staticmethod
    def pixel_addr(x, y):
        """Translate an x,y coordinate to a pixel index."""
        if y == 0:
            if x < 7:
                return 118 - x
            if x < 15:
                return 141 - x
            if x < 21:
                return 106 + x
            if x == 21:
                return 15
            return x - 14

        if y == 1:
            if x < 2:
                return 69 - x
            if x < 7:
                return 86 - x
            if x < 12:
                return 28 - x
            if x < 14:
                return 45 - x
            if x == 14:
                return 47
            if x == 15:
                return 41
            if x < 21:
                return x + 9
            if x == 21:
                return 95
            if x < 26:
                return x + 67
            return x + 50

        if x == 0:
            return 85
        if x < 7:
            return 102 - x
        if x < 11:
            return 44 - x
        if x < 14:
            return 61 - x
        if x == 14:
            return 63
        if x < 17:
            return 42 + x
        if x < 21:
            return x + 25
        if x == 21:
            return 111
        if x < 27:
            return x + 83
        return 93

dhalbert commented 3 years ago

I much prefer the table. You could make it even more compact and faster by turning it into a bytes and indexing into it "by hand" (compute the offset yourself), but that is probably unnecessary here.

To prevent pylint from complaining about duplicate code, or anything in particular, you should be able to do a # pylint: disable=... for the check in question. I don't know how to fix that for duplicate code. Don't go to extremes just to satisfy its whims; override it if it's being unreasonable.

dglaude commented 3 years ago

Previously @ladyada said: "please at least make this a tuple, or a bytearray, - at best please algorithize as this table will take up valuable ram on small devices": https://github.com/adafruit/Adafruit_CircuitPython_IS31FL3731/pull/26#discussion_r403727405

I guess I did not get the "tuple" nor the "bytearray" part of the answer and went algorithm... and that was a nightmare result. Also that remark was before the library was split in module, so the memory issue was different.

I'll give this another try and pylint will not detect that as duplicate.

tannewt commented 3 years ago

bytes or bytearrays are the smallest option because they won't have tuple overhead and every element will be 1 byte instead of 4 like all Python objects are.

dglaude commented 1 year ago

The question came in the implementation of #41 and 5x5 support was added with #45 . Not sure about the implementation details, but this issue is not useful anymore.

adafruit / Adafruit_CircuitPython_IS31FL3731

Do you prefer a lookup table or an algorithm? + duplicate code #43