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

[Paul Elder]

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)


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
> > >