Issue 95: Added PixelFormat class, replaced usages of color formats#121
Issue 95: Added PixelFormat class, replaced usages of color formats#121suppresseddev wants to merge 2 commits intomeetecho:mainfrom
Conversation
…dded imports when necessary.
|
Hey, thanks a lot for the contribution! The direction is right and the work is appreciated. 🙌 That said, I'd like to request a few changes before we can merge this. 1. Format normalization inside the class The whole point of this class is to abstract away the differences between the various nodes — so consumers don't have to care about implementation details of each one. A couple of concrete cases already in the codebase:
This inconsistency should be handled internally by the class rather than leaking out. I'd suggest adding a small normalization step — a private method that canonicalizes format strings before any comparison or dispatch: @staticmethod
def _normalize_format(fmt: str) -> str:
"""Canonicalize format strings to a consistent internal representation."""
return fmt.strip().lower()Then use 2. Update the affected nodes accordingly Both 3. Consider using Since this class is likely to be instantiated frequently (once per node, potentially many times during a pipeline run), it's worth declaring 4. Remove inline comments Please strip out the inline comments — the code should be self-explanatory, and anything worth explaining belongs in the docstrings or the PR description. Comments that restate what the code does add noise without value. 5. Ruff linting is failing The ruff workflow is currently failing on this PR. There are a few small things to fix — the exact rules and how to run the linter locally are documented in These changes together would make the abstraction complete and robust. Happy to discuss any of them — feel free to ping me! |
|
@suppresseddev Hi, are you still around? Have you had a chance to make any progress on this? |
|
Oh I can do that! Sorry this is my first time doing this ^^; Will get it done asap (today) |
|
@suppresseddev Absolutely no problem, we’re happy to help you on your first ride if you need anything! |
…inputted color formats, replaced additional usages of hardcoded color formats
|
Sorry for the delay, got super side tracked with university. Your recommendations have been applied. I'm not entirely certain the ruff-linting worked? It made some changes with quotes, but I'm not sure if that is what you meant for me to do. |
|
Thank you again @suppresseddev for the changes in your PR! I still remember being flooded with uni stuff, so well done on that! I reviewed your code, and there are still a number of things that need some attention. The most important one is that Thinking about it, having a class storing formats so that they can be uniformed across components does not really require constantly instantiating new objects. This leaves several options:
Deciding which option to go for is a matter of performance, usability and extensibility. On this regard, I tested some options to see where we stand in terms of access time and conversion time. I imagine these options could be implemented like this (I excluded the named tuple, as there is no real point in having that as it would still be absorbed by the module option): from enum import StrEnum
from typing import ClassVar
from dataclasses import dataclass
# this is a bare class with only class variables
class FormatClass:
@classmethod
def normalize(cls, fmt: str) -> str:
if not hasattr(cls, fmt.strip().upper()):
return ""
return fmt.strip().lower()
RGB: ClassVar[str] = "rgb"
BGR: ClassVar[str] = "bgr"
...
# this is a dataclass with class variables
@dataclass
class FormatDataclass:
@classmethod
def normalize(cls, fmt: str) -> str:
if not hasattr(cls, fmt.strip().upper()):
return ""
return fmt.strip().lower()
RGB: ClassVar[str] = "rgb"
BGR: ClassVar[str] = "bgr"
...
# this is a string enum
class FormatEnum(StrEnum):
@classmethod
def normalize(cls, fmt: str) -> str:
if not hasattr(cls, fmt.strip().upper()):
return ""
return fmt.strip().lower()
RGB = "rgb"
BGR = "bgr"
...The module option might look like this: # this is a separate format_module.py file
RGB = "rgb"
BGR = "bgr"
BGRA = "bgra"
YUV420P = "yuv420p"
YUV444P = "yuv444p"
YUV422 = "yuv422"
GRAY = "gray"
PAL8 = "pal8"
_formats = [RGB, BGR, BGRA, YUV420P, YUV444P, YUV422, GRAY, PAL8]
def normalize(fmt):
if fmt.strip().upper() not in _formats:
return ""
return fmt.strip().lower()I then measured the times: import timeit
iterations = 2_000_000
print("class access")
print(timeit.timeit(lambda: FormatClass.RGB, number=iterations))
print("class conversion")
print(timeit.timeit(lambda: FormatClass.normalize("RGB"), number=iterations))
print()
print("dataclass access")
print(timeit.timeit(lambda: FormatDataclass.RGB, number=iterations))
print("dataclass conversion")
print(timeit.timeit(lambda: FormatDataclass.normalize("RGB"), number=iterations))
print()
print("enum access")
print(timeit.timeit(lambda: FormatEnum.RGB, number=iterations))
print("enum conversion")
print(timeit.timeit(lambda: FormatEnum.normalize("RGB"), number=iterations))
print()
print("module access")
print(timeit.timeit(lambda: format_module.RGB, number=iterations))
print("module conversion")
print(timeit.timeit(lambda: format_module.normalize("RGB"), number=iterations))Execution times follow: This seems to suggest that the module option would be the fastest. Using a module eliminates the need for a separate object holding formats and is quite portable. Of course, there is no safety on immutability, but the same is true for every other python module in existance! A small safeguard/reminder could be added defining those variables as final: from typing import Final
RGB: Final[str] = 'rgb'
...I would very much like to hear your opinion on this, as well as @GaijinKa 's, so that we can decide what is the best approach to take here. As for the other issues with the PR, we have a number of automated checks in place that are executed whenever new code is submitted. One of these is the linting, a way of making sure every line of code respects a set of rules defined on the project. We use
If you are considering keep up with contributing (which we really look forward to!), a great starting point is the contributing guidelines at the root of the project! |
|
How do I use Ruff correctly with PyCharm, i have something running that did some auto fixes but it seems like it didn't encapsulate all of the changes you guys wanted. |
|
I am not 100% sure about PyCharm as I don't use it, but according to what I found online I believe they added native support for ruff at some point. If you are working with virtual environments, install ruff with Hope this helps! |
Description
In reference to issue 95, I created the requested PixelFormat class and updated uses of pixel formatting with references to the PixelFormat class.
PR type
Select all the labels that apply to this PR.
Key modifications and changes
Added juturna/names/_pixel_format.py, which contains the PixelFormat class.
Added the PixelFormat class to the package juturna.names.
Replaced usages of pixel formats with references to the PixelFormat class.
Affected components
Video Handling: Specifically, the things found under
juturna/nodes/sourceAdditional context
N/A