worldcoin / open-iris

Open Iris Recognition Inference System (IRIS)
MIT License
222 stars 42 forks source link

Bugfix/callbacks #18

Closed brgrp closed 4 months ago

brgrp commented 5 months ago

Callbacks are not always forwarded to base class

PR description

This pull request addresses an issue where custom callbacks were not being forwarded to the base class during initialization, leading to potential issues with the functionality of the affected classes.

Issue

The problem arises when implementing custom callbacks, as they are not automatically passed to the base class during initialization. This behavior can cause inconsistencies in the behavior of the classes involved.

For an example implementation see: https://github.com/brgrp/open-iris/tree/38a101af5091004df20b13d025b40db4053c0a31/examples

Solution

To resolve this issue, I have modified the code to forward callbacks to the underlying base class, specifically the Algorithm class. Additionally, I have implemented a check to ensure that callbacks are properly handled when they are None or empty.

Limitations

Currently, this fix has only been applied to two classes: MultilabelSegmentationBinarization and ONNXMultilabelSegmentation. To fully address the issue, it needs to be extended to other classes where callbacks are required.

Quick side note: I gave the pre-commit script a shot, but it seemed like ruff wasn't quite on board with it! So, I decided to give ruff and blake a manual run instead. Fingers crossed that everything went smoothly! Thanks for your understanding.

Type

Checklist

brgrp commented 4 months ago

Hey @wiktorlazarski that's great news!

Unfortunetly I am not able to merge as only user with write access to the repo are able to do.

This branch has no conflicts with the base branch Only those with [write access](https://docs.github.com/articles/what-are-the-different-access-permissions) to this repository can merge pull requests.

Happy to catch up on this challange soon.

wiktorlazarski commented 4 months ago

@brgrp, I'm sorry, I forgot about that. I'll merge it for you. Thanks.