Feature/app restructure

[davisadam10]

Hi Guys,

So I have a branch and a pull request open, this is NOT a candidate to be merged yet, however wanted to put this out there so that you guys can take a look at the changes and understand the direction I started taking to split this out into a more modular approach.

The good news is thomas unit tests still pass - so yay The ban news is the UI is currently broken because 1) time and 2) im not overly familiar with the UI code.

So the changes as an explanation and concept as follows

1) The idt module now has a few sub packages

core - This is the core of the idt, it contains modules for common - generic generator functions utilities - functions and helpers constants - a module to hold call the constants used across the project so we can change the values in a single place if needed structures - a module to hold some generic and helper data structures and objects that are used mainly in the new project_settings and application modules (later)

framework - This module holds the project_settings module and any other modules that we would require in future which are critical objects which are needed during the application execution or framework logic

generators - This module holds the generators for the idt project, currently there is a base generator which holds all of what i think is the generic generator logic. There is also now the prosumer_camera generator which inherits from the base generator and has the specific logic for the prosumer camera only.

application - The main application which can be run headless to load and save, create project_settings, select a generator and run the execution logic. External tools or UIs should only interact with and via the application, if we need to perform any calculations which are based on the results of the generator run, they should happen in the application.

2) idt_metadata_property & IDTMetaData

One of the key new structures is the introduction of the IDTMetaData, this is a simple structure which allows us to store metadata in regards to a param within the project, it allows us to store the default value, a description, as well as a UI_type and category.

Each of the params is defined as a constant

The idt_metadata_property is a decorator which allows us to create properties on a class which allow for standard getter and setter functionality, but also allow us to do

property.metadata

This approach allows us to define objects in the core, decorate them with metadata, which can be read by the application layer, of a UI implementing atop the application to dynamically understand the properties, and how they need to be displayed in the UI.

eg. you can build a small UI component for each ui type say a STRING_FIELD, and the ui can dynamically add the correct type of field, display name, grouping etc

The idt_metadata_property are used in the IDTProjectSettings class, but these could also be used on the generators them selves if needed, this would allow us to dynamically display any custom properties that a generator may have.

Ideally we dont need many per generator params but the option is there if needed.

2) IDTProjectSettings

The IDRProjectSettings has replaced the hard coded dict schema for the json files which are created or processed.

This is a simple class which holds the schema, there is a property for each field of data which is needed in the UI, and through the framework, these properties are all decorated with idt_metadata_property so the ui can tell what to draw.

The IDRProjectSettings can serialize to json on disk, and also load back from json files, this is all due to the inheritance of a base serializable object that handles the.

3) IDTApplication

The application as mentioned is the main entry point for the developer or user, it allows the user to get a list of the available generators, and holds a project_settings object within in.

The application now holds much of the generic execution logic, such as the extract_archive, and the from_archive and from_specification (now from_project_settings) methods which previously lived on the generator.

Similarly the zip function is also housed here, however there are a number of TODOs called out in the code as there are some challenges relating to this and other.

4) What should be in the project_settings, what should be in the application, what should be in the generator? The project_settings (previously the specification) should be able to serialize to disk, load and run and produce the exact same result each time.

Right now there are a number of options, which seem to be passed from the ui which do not exist in the spec, these are currently being set on the application itself as transient data that the generators can access.

Secondly there is some data being calculated in the UI itself, such as deltaEs etc, these are then passed back through to the zip function on the generator, which previously was working if not ideal, now we have the abstraction layer between ui and generators, these calculations should be moved to the application so the ui is just a ui.

5) Updating the UI

Updating the UI should be relatively straight forward, as right now it creates an instance of the IDTGenerator and calls the functions directly. This should just require this to be replaced with the IDTApplication instance and call the framework functions instead, once the 4) additional computation steps are moved to the application.

6) DocStrings & Unit Tests

UnitTests have been moved outside the idt module and are along side it. Thomas original unit tests have been ported over and the exact same tests are passing, so this is a good sign i didn't break anything.

The docstrings are a little bit all over the place now, as i have done some pretty major surgery chopping files into pieces and moving things around, so these will need to be all updated once we get this thing back on its feet.

With a bit of help on the UI side and moving that rouge computation down to the application level i think this is pretty close to being back to where it was, but with the option to extend it, and make it less monolithic.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[KelSolaar]

Hello @davisadam10,

Thanks for all the work! I tried to checkout the PR but unfortunately, we burnt through all the repo quota here:

git-lfs/3.4.1 (GitHub; darwin arm64; go 1.21.5)
git version 2.39.2

$ git-lfs filter-process
Error downloading object: tests/resources/PTZ_100_NOT-REGULAR_STOPS.zip (6e95e2b): Smudge error: Error downloading tests/resources/PTZ_100_NOT-REGULAR_STOPS.zip (6e95e2bbd6006fcc77e1e4dcb69725b1449593c6f594467b1af59c310d03306b): batch response: This repository is over its data quota. Account responsible for LFS bandwidth should purchase more data packs to restore access.

This repository is over its data quota. Account responsible for LFS bandwidth should purchase more data packs to restore access.

I avoided having to use LFS by generating relatively lightweight synthetic data. If we want actual production data, we should probably consider to have a separate repository that we can sub-module maybe, or another space we can pull from during the tests?

Cheers,

Thomas

[davisadam10]

@KelSolaar ah...

There's always some thing 🤦

So

1) can we put a few dollars in the meter so we can at least checkout and then clean up as/if/when needed.$5 a month gives us 50GB of storage..

2) we should think about where we would store some more production based examples if we don't want to use lfs, as handling the pre linearised generator , and the s curve tone mapping generator (we need better names ) for the ptz camera will want to be unit tested against a few production samples from a few cameras to make sure we don't break anything as we go.

3) I generally have used lfs for this as you can pull the code without doing the lfs pull

GIT_LFS_SKIP_SMUDGE=1 git checkout/pull

And stops the actual hit repo getting very big with binary file revisions.

Happy to host this stuff elsewhere if preferred

[aforsythe]

Would be preferable to host data somewhere other than Github / git-lfs. I know this sounds like a ridiculous hoop, but I have no direct way to pay for things here.

[KelSolaar]

Hello,

I started to take a better look, here are a few initial thoughts:

I quite like the IDTProjectSettings, this will be helpful!

What should be in the project_settings, what should be in the application, what should be in the generator? The project_settings (previously the specification) should be able to serialize to disk, load and run and produce the exact same result each time.

I'm actually not sure to understand the current rational for splitting the BaseGenerator / IDTApplication.

Now we have to do things like that in BaseGenerator:

        self._samples_analysis = deepcopy(self._application.project_settings.data)
        # Baseline exposure value, it can be different from zero.
        if 0 not in self._application.project_settings.data[DataFolderStructure.COLOUR_CHECKER]:
            EVs = sorted(self._application.project_settings.data[DataFolderStructure.COLOUR_CHECKER].keys())

        if self._application.cleanup:
            shutil.rmtree(self._application.working_directory)

where before everything was mostly encapsulated inside the generator.

This also creates that kind of not very elegant way of using the code:

        idt_application = IDTGeneratorApplication()
        idt_application.idt_generator = "IDTGeneratorProsumerCamera"
        archive = os.path.join(self.get_test_resources_folder(), "synthetic_001.zip")
        idt_application.working_directory = idt_application.extract_archive(archive)
        ....

Where before:

    idt_generator = IDTGeneratorProsumerCamera.from_archive(
        resources_directory / "synthetic_001.zip", cleanup=True
    )

or :

    idt_generator = IDTGeneratorProsumerCamera.from_specification(
        specification, resources_directory / "synthetic_001"
    )

Regarding IDTGeneratorProsumerCamera vs BaseGenerator, at the point where we are doing an inheritance scheme, we should use an ABC so that we can do

    @abstractmethod
    def generate_LUT(self):
        ...

rather than

    def generate_LUT(self):
        raise NotImplementedError("generate_LUT method not implemented")

We should also make sure that the critical properties/attributes are all in the ABC, for example:

        self._LUT_unfiltered = None
        self._LUT_filtered = None
        self._LUT_decoding = None

might not need to be in the ABC and maybe we would those to be reduced:

• sample
• sort
• generate_LUT
• filter_LUT
• decode
• optimise

to maybe this

• sample
• sort
• generate_LUT (includes filter_LUT)
• decode
• optimise

It is also worth noting that we have added almost 4800 lines of codes and with docstrings this will be ballooning making maintenance harder. It seems like you are not using pre-commit hooks, I would recommend that you set them up.

Cheers,

Thomas

[KelSolaar]

Oh and to install to the pre-commit hooks: pre-commit install assuming you have installed the repo with Poetry

You can also bypass the hooks when committing, you can do a git commit --no-verify -a -m "My message..."