[PATCH v1 2/2] utils: tuning: Change Tuner.add() to accept a list of modules

[Stefan Klug]

On Wed, Aug 14, 2024 at 05:00:17PM +0900, Paul Elder wrote:
> On Mon, Aug 05, 2024 at 10:55:23AM +0200, Stefan Klug wrote:
> > Hi Kieran,
> > 
> > Thank you for the review.
> > 
> > On Mon, Aug 05, 2024 at 08:39:04AM +0100, Kieran Bingham wrote:
> > > Quoting Stefan Klug (2024-08-05 08:06:33)
> > > > Change the first parameter of Tuner.add() to be a list of modules
> > > > instead of a single module. This allows more compact code and is in sync
> > > > with Tuner.set_output_order().
> 
> Hm true. I got the idea from my university days of working with
> pytorch...
> 
> > > > 
> > > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > > ---
> > > >  utils/tuning/libtuning/libtuning.py   |  4 ++--
> > > >  utils/tuning/raspberrypi_alsc_only.py |  2 +-
> > > >  utils/tuning/rkisp1.py                | 10 +---------
> > > >  3 files changed, 4 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/utils/tuning/libtuning/libtuning.py b/utils/tuning/libtuning/libtuning.py
> > > > index e7c63535fefd..a167e39ef08e 100644
> > > > --- a/utils/tuning/libtuning/libtuning.py
> > > > +++ b/utils/tuning/libtuning/libtuning.py
> > > > @@ -94,8 +94,8 @@ class Tuner(object):
> > > >          self.config = {}
> > > >          self.output = {}
> > > >  
> > > > -    def add(self, module):
> > > > -        self.modules.append(module)
> > > > +    def add(self, modules):
> > > > +        self.modules.extend(modules)
> > > 
> > > I guess it already supported a list, it's just we're only calling a
> > > single module and it had only been used that way.
> > 
> > Not really, it now calls extend() instead of append() on the list.
> > 
> > > 
> > > The pluralisation here is certainly worthwhile though to convey this.
> > > 
> > > >  
> > > >      def set_input_parser(self, parser):
> > > >          self.parser = parser
> > > > diff --git a/utils/tuning/raspberrypi_alsc_only.py b/utils/tuning/raspberrypi_alsc_only.py
> > > > index 777d800765e0..fe2b5b8b5a24 100755
> > > > --- a/utils/tuning/raspberrypi_alsc_only.py
> > > > +++ b/utils/tuning/raspberrypi_alsc_only.py
> > > > @@ -14,7 +14,7 @@ from libtuning.generators import RaspberryPiOutput
> > > >  from raspberrypi.alsc import ALSC
> > > >  
> > > >  tuner = lt.Tuner('Raspberry Pi (ALSC only)')
> > > > -tuner.add(ALSC)
> > > > +tuner.add([ALSC])
> > > 
> > > Does this one need to be changed though? (It doesn't hurt I believe), I
> > > see we're in a module called _alsc_only.py, so this won't be expected to
> > > be added to..
> > 
> > As noted above, this is actually required (If we don't want to go for
> > more magic).
> 
> Wouldn't the only magic be isinstance(modules, list)? (which tbh I'd
> prefer but idk if there's much demand for both)

Yes, that would be the magic. I'll implement it and post a v2.

Cheers,
Stefan

> 
> 
> Paul
> 
> > 
> > > 
> > > But still ... it keeps the usages consistent...
> > > 
> > > >  tuner.set_input_parser(RaspberryPiParser())
> > > >  tuner.set_output_formatter(RaspberryPiOutput())
> > > >  tuner.set_output_order([ALSC])
> > > > diff --git a/utils/tuning/rkisp1.py b/utils/tuning/rkisp1.py
> > > > index 5d7a69fc4a13..f5c42a61d15e 100755
> > > > --- a/utils/tuning/rkisp1.py
> > > > +++ b/utils/tuning/rkisp1.py
> > > > @@ -45,15 +45,7 @@ lsc = LSCRkISP1(debug=[lt.Debug.Plot],
> > > >                  smoothing_function=lt.smoothing.MedianBlur(3),)
> > > >  
> > > >  tuner = lt.Tuner('RkISP1')
> > > > -tuner.add(agc)
> > > > -tuner.add(awb)
> > > > -tuner.add(blc)
> > > > -tuner.add(ccm)
> > > > -tuner.add(color_processing)
> > > > -tuner.add(filter)
> > > > -tuner.add(gamma_out)
> > > > -tuner.add(lsc)
> > > > -
> > > > +tuner.add([agc, awb, blc, ccm, color_processing, filter, gamma_out, lsc])
> > > 
> > > 
> > > Keeping them on separate lines would make it easier to add new entries
> > > ... but there's a finite number of items we'll add here and once
> > > 'feature complete' this line won't change - so it's not quite comparable
> > > to other lists that we might otherwise extend, so :
> > > 
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > 
> > > >  tuner.set_input_parser(YamlParser())
> > > >  tuner.set_output_formatter(YamlOutput())
> > > >  tuner.set_output_order([agc, awb, blc, ccm, color_processing,
> > > > -- 
> > > > 2.43.0
> > > >