Skip to content

Histogram blocks refactored#25

Open
patlevin wants to merge 3 commits intomahmoudnafifi:masterfrom
patlevin:hist-block-refactored
Open

Histogram blocks refactored#25
patlevin wants to merge 3 commits intomahmoudnafifi:masterfrom
patlevin:hist-block-refactored

Conversation

@patlevin
Copy link
Copy Markdown

Histogram Block Refactoring

Motivation

When using histogram loss, loss calculation can take up a significant portion of the training process.
This PR tries to improve on that by refactoring the original implementation of the losses while staying
consistent with the output of the original implementations.

Content

This PR contains refactored histogram losses. It adds a common base class to make future additions simpler as well as to
reduce code duplication. Common functionality, such as the kernel methods for resizing (sampling), pixel counting, and intensity scaling have been extracted into functions found in the new base class module.

Results

The refactored histogram blocks are now significantly smaller and easier to understand (at least in my opinion🤔).
The public interface and default parameters have been kept as-is, so no changes should be required on the caller side.
Performance tests showed an improvement of >5x for each RGB-uv, rg-chroma, and Lab blocks using their respective default parameters.

This performance improvement only increases on non-datacentre class hardware, such as consumer and professional GPUs.
The performance uplift on consumer grade hardware has been observed to be up to 20x due to hardware limitations of these devices (primarily due to the very limited double precision support).

Accuracy w.r.t. to the original implementation is acceptable. In all but some synthetic tests, mean deviation between original and refactored versions are well within the machine epsilon of 32-bit floating point maths.

Limitations

The refactored version drops support for input images bigger than about 2.1 gigapixels (i.e. roughly 45,000 by 45,000 pixels).
I personally don't think this limitation will pose an issue in the foreseeable future, but I think it's worth mentioning nonetheless😉.

And finally: thank you for your great work and for publishing your code!

@mahmoudnafifi
Copy link
Copy Markdown
Owner

Hi @patlevin,

Thank you so much for your contribution and all the effort in testing and explaination. I am quite busy those two weeks and I will do my best to check the PR soon.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants