search

tfaehse / DashcamCleaner

Censor identifiable information in videos, in particular dashcam recordings in Germany.
GNU Affero General Public License v3.0
131 stars 27 forks source link

detections class to separate bounds box and other data for detections stored separately from them #30

Closed joshinils closed 2 years ago

joshinils commented 2 years ago
joshinils commented 2 years ago

I would like to change the parameters on the VideoBlurrer to something else. https://github.com/tfaehse/DashcamCleaner/blob/2a425fd8cd5dbb91b461ab734c9e420431bb921e/dashcamcleaner/main.py#L49 is a call without parameters and thus requires the default parameter of None in VideoBlurrer (which I want to get rid of) and later sets the parameters here: https://github.com/tfaehse/DashcamCleaner/blob/2a425fd8cd5dbb91b461ab734c9e420431bb921e/dashcamcleaner/main.py#L116 which i feel like is invading the VideoBlurrers private variables.

for those reasons i think a separate named entity (i.e. a class) for the parameters is best, what do you say @tfaehse ?

tfaehse commented 2 years ago

That sounds like a good idea. I've implemented some good old inheritance, that also makes the wrapper more of... a wrapper. The parameters are still overwritten (since they are very likely to change between launching the GUI and clicking start), but the alternative is making them private self._parameters.. and adding a setter that can set them whenever anyway, so I guess this works for now. The additional perk is that there's less duplicate code between the blurrer and the qt-wrapper now.

A dictionary for parameters seems okay to me, a potential replacement (@dataclass for example?) wouldn't really offer much of a change I think.

As for dbe5b20: Did you try that out? For me tqdm's predictions were all over the place, because with a batch size of e.g. 16 there are 15 super quick iterations (causing predictions to be faster and faster), but then the next (very slow) iterations causes it to go up again, and so on.

joshinils commented 2 years ago

That sounds like a good idea. I've implemented some good old inheritance, that also makes the wrapper more of... a wrapper. The parameters are still overwritten (since they are very likely to change between launching the GUI and clicking start),

Ye that makes sense, I just didn't like the None parameter.

but the alternative is making them private self._parameters.. and adding a setter that can set them whenever anyway, so I guess this works for now.

That would work, but would probably involve too much boilerplate? Is at least a good argument against it.

I was however thinking that a separate class could be used which can then involve type-hints and checking argument-bounds. That way one can rely on the arguments being the right type and in-range. Then the VideoBlurer would not need separate variables, just a single object of one known and well-defined type which groups all arguments (logically) together. Avoiding things like: https://github.com/tfaehse/DashcamCleaner/blob/ad3039aeed42b5ba53769282763e844cf8b6f360/dashcamcleaner/src/blurrer.py#L37-L41

The additional perk is that there's less duplicate code between the blurrer and the qt-wrapper now.

A dictionary for parameters seems okay to me, a potential replacement (@DataClass for example?) wouldn't really offer much of a change I think.

Agree.

As for dbe5b20: Did you try that out?

I did now :eyes:.

For me tqdm's predictions were all over the place, because with a batch size of e.g. 16 there are 15 super quick iterations (causing predictions to be faster and faster), but then the next (very slow) iterations causes it to go up again, and so on.

Not for me, the prediction goes down each batch that is saved. If it is such a different behaviour when using GPU, I will revert that now.