fix scaling of RGBs for tensorflow/keras

[candemircan]

Hi,

This pull request fixes the following:

Harmonizer (and Generic tf) Models

The default tf preprocessing now rescales the RGBs between 0, and 1. As you can see here, now we get identical results from thingsvision and when using the harmonized models directly.

Note that this also applies the same scaling to any tf model that is manually turned into an extractor, and any tf model that does not specify a preprocessing function.

tf.keras pretrained model fixes

Before these commits, always the same preprocessing was applied to the pretrained tf.keras.applications models. This yielded silent but catastrophic errors, as unfortunately, the pretrained models differ in their preprocessing in non-systematic ways. This is now fixed. Again, using models via thingsvision and directly yielded the same results here.

Getting the correct preprocessing function is a bit tricky, as it is done via different modules and functions and not through arguments (e.g., tf.keras.applications.efficientnet_v2.preprocess_input). Additionally, the preprocessing module names differ from the model names in subtle ways. I tried to resolve this using regular expressions, and it seems to work for the current models. If there is no regexp match, currently we raise a warning saying no preprocessing function could be found and that the user should be careful, and we revert to default preprocessing described in the previous section. If new pretrained models get added, its name needs to be added to the function. This is not ideal, but I couldn't find a better, more generic way.

Additionally, different keras models have different input sizes, and this needs to be accounted for. Now it should be sorted.

testing

I did all the testing in a separate repo here, where I'd also reproduced some of these issues. I know you have a tests folder here. I couldn't quite wrap my head around how to do some of the simple tests I did there. For example, I see that you have a create_test_images function that you call, but I don't see where these images actually get used in the tests. Instead, I think some manually created values are being passed through the models? Either way, it should be straightforward to add the tests I did into your test suite for someone who understands the structure.

---

Sorry for closing and reopening an issue. Had to create a new branch and delete the old one.

[fel-thomas]

Hi all,
this is perfect—thank you @candemircan for handling this and @LukasMut for the management of the issue!
The reproduction is really convincing.

Wishing you both a wonderful week!

[fel-thomas]

LGTM

[LukasMut]

Hi all,

this is perfect—thank you @candemircan for handling this and @LukasMut for the management of the issue!

The reproduction is really convincing.

Wishing you both a wonderful week!

Thanks for taking a look @fel-thomas! 🙏🏼 You too have a great rest of the week :)

[LukasMut]

Hi,

This pull request fixes the following:

Harmonizer (and Generic tf) Models

The default tf preprocessing now rescales the RGBs between 0, and 1. As you can see here, now we get identical results from thingsvision and when using the harmonized models directly.

Note that this also applies the same scaling to any tf model that is manually turned into an extractor, and any tf model that does not specify a preprocessing function.

tf.keras pretrained model fixes

Before these commits, always the same preprocessing was applied to the pretrained tf.keras.applications models. This yielded silent but catastrophic errors, as unfortunately, the pretrained models differ in their preprocessing in non-systematic ways. This is now fixed. Again, using models via thingsvision and directly yielded the same results here.

Getting the correct preprocessing function is a bit tricky, as it is done via different modules and functions and not through arguments (e.g., tf.keras.applications.efficientnet_v2.preprocess_input). Additionally, the preprocessing module names differ from the model names in subtle ways. I tried to resolve this using regular expressions, and it seems to work for the current models. If there is no regexp match, currently we raise a warning saying no preprocessing function could be found and that the user should be careful, and we revert to default preprocessing described in the previous section. If new pretrained models get added, its name needs to be added to the function. This is not ideal, but I couldn't find a better, more generic way.

Additionally, different keras models have different input sizes, and this needs to be accounted for. Now it should be sorted.

testing

I did all the testing in a separate repo here, where I'd also reproduced some of these issues. I know you have a tests folder here. I couldn't quite wrap my head around how to do some of the simple tests I did there. For example, I see that you have a create_test_images function that you call, but I don't see where these images actually get used in the tests. Instead, I think some manually created values are being passed through the models? Either way, it should be straightforward to add the tests I did into your test suite for someone who understands the structure.

---

Sorry for closing and reopening an issue. Had to create a new branch and delete the old one.

Thanks a lot for handling this @candemircan! 🙏🏼 Much appreciated!

[LukasMut]

@martinhebart could you make sure that this PR gets into a state where I can confidently merge it into main? It would be very helpful if one of you @jonasd4 @Alxmrphi @andropar could review this PR. I'll then do the final review and accept the changes.

[andropar]

I'm no expert with Keras, but looks good to me! The test does speak for itself too, good job 👍

[LukasMut]

LGTM. I cleaned up a bit and have one minor comment. Overall this looks good. Great work!

[LukasMut]

Why re.match(pattern, string) and not re.search(pattern, string)? re.match searches for the pattern only at the beginning of a string, whereas re.search searches through the entire string. I am seeing that you specify start and end positions using ^ and $ although you use re.match. Why is that? I think we could just drop all ^ and $ and it would work equally well.

[candemircan]

sure, if you're confident that it'll be identical in behaviour then go ahead

[andropar]

Wouldn't omitting $ cause unwanted matches (e.g. "MobileNet" would match "MobileNetV3" too)?

[LukasMut]

@candemircan This was just feedback :) I am trying to understand whether there is a specific reason for re.match in combination with the regular expressions you used :)

@andropar I guess it depends on re.match vs re.search. re.match searches for the pattern only at the beginning of a string. Thus, ^ is baked into the function and $ is omitted. I don't think we ever need ^ (definitely not for re.match) but $ is needed for one of the two (or maybe even for both the more I think about it? 🤔)

[candemircan]

no specific reason on my side. it was the first thing I wrote that worked correctly, and I left it as is because I don't know regexp very well. If there is a better way to do this, go ahead and change it please :)

[LukasMut]

Ok, thanks for the clarification! I changed it from re.match to re.search because ^ is redundant with re.match. It's baked into the function. I left the patterns as you specified them to be as explicit as possible ($ is indeed necessary; I just tested it). Thank you for helping with this! I will accept the PR now and merge it into main.

[candemircan]

coolio, thanks Lukas! all the best