| Message ID | 20240805070644.1536232-3-stefan.klug@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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(). > > Signed-off-by: Stefan Klug <stefan.klug@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. 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.. 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@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 >
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(). > > > > Signed-off-by: Stefan Klug <stefan.klug@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). Regards, Stefan > > 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@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 > >
Quoting Stefan Klug (2024-08-05 09:55:23) > 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(). > > > > > > Signed-off-by: Stefan Klug <stefan.klug@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. Ohh I completley missed that ! > > > > 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). Yup. It's clearer now thanks. > > Regards, > Stefan > > > > > 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@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 > > >
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@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@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 > > >
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@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@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 > > > >
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) 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]) 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]) tuner.set_input_parser(YamlParser()) tuner.set_output_formatter(YamlOutput()) tuner.set_output_order([agc, awb, blc, ccm, color_processing,
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(). Signed-off-by: Stefan Klug <stefan.klug@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(-)