[3/3] rteval: systopology: Slight CpuList.__expand_cpulist() cleanup

This method currently aggregates CPUs into a list, then converts this to
set and then back to list. The aggregation can instead be done directly
into a set.

(as an offside, it would make more sense for CpuList to have its storage be
a set in the first place as duplicate CPU ids don't make sense for it, but
that's a separate discussion :-))

The integer conversion of the "a-b" pattern can also be condensed into a
single map() expression.

Signed-off-by: Valentin Schneider <vschneid@redhat.com>
---
 rteval/systopology.py | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

On Tue, 19 Apr 2022, Valentin Schneider wrote:

> This method currently aggregates CPUs into a list, then converts this to
> set and then back to list. The aggregation can instead be done directly
> into a set.
> 
> (as an offside, it would make more sense for CpuList to have its storage be
> a set in the first place as duplicate CPU ids don't make sense for it, but
> that's a separate discussion :-))
> 
> The integer conversion of the "a-b" pattern can also be condensed into a
> single map() expression.
> 
> Signed-off-by: Valentin Schneider <vschneid@redhat.com>
> ---
>  rteval/systopology.py | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/rteval/systopology.py b/rteval/systopology.py
> index b2da7bb..2a28f9c 100644
> --- a/rteval/systopology.py
> +++ b/rteval/systopology.py
> @@ -102,20 +102,19 @@ class CpuList:
>          """ expand a range string into an array of cpu numbers
>          don't error check against online cpus
>          """
> -        result = []
> -
>          if not cpulist:
> -            return result
> +            return []
> +
> +        result = set()
>  
>          for part in cpulist.split(','):
>              if '-' in part:
> -                a, b = part.split('-')
> -                a, b = int(a), int(b)
> -                result.extend(list(range(a, b + 1)))
> +                a, b = map(int, part.split('-'))
> +                result |= set(range(a, b + 1))
>              else:
>                  a = int(part)
> -                result.append(a)
> -        return [int(i) for i in list(set(result))]
> +                result |= {a}
> +        return list(result)
>  
>      def getcpulist(self):
>          """ return the list of cpus tracked """
> -- 
> 2.27.0
> 
> 

I'm guessing that the reason for the "set" was to remove any potential 
duplicates. Duplicates are not normally a problem in rteval, but if you 
want to ensure you handle any user input correctly, it makes sense to do 
that. I think your code is correct, but I'm not sure if it buys us 
anything.

John

On 29/04/22 16:53, John Kacur wrote:
> On Tue, 19 Apr 2022, Valentin Schneider wrote:
>
>> This method currently aggregates CPUs into a list, then converts this to
>> set and then back to list. The aggregation can instead be done directly
>> into a set.
>>
>> (as an offside, it would make more sense for CpuList to have its storage be
>> a set in the first place as duplicate CPU ids don't make sense for it, but
>> that's a separate discussion :-))
>>
>> The integer conversion of the "a-b" pattern can also be condensed into a
>> single map() expression.
>>
>> Signed-off-by: Valentin Schneider <vschneid@redhat.com>
>> ---
>>  rteval/systopology.py | 15 +++++++--------
>>  1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/rteval/systopology.py b/rteval/systopology.py
>> index b2da7bb..2a28f9c 100644
>> --- a/rteval/systopology.py
>> +++ b/rteval/systopology.py
>> @@ -102,20 +102,19 @@ class CpuList:
>>          """ expand a range string into an array of cpu numbers
>>          don't error check against online cpus
>>          """
>> -        result = []
>> -
>>          if not cpulist:
>> -            return result
>> +            return []
>> +
>> +        result = set()
>>
>>          for part in cpulist.split(','):
>>              if '-' in part:
>> -                a, b = part.split('-')
>> -                a, b = int(a), int(b)
>> -                result.extend(list(range(a, b + 1)))
>> +                a, b = map(int, part.split('-'))
>> +                result |= set(range(a, b + 1))
>>              else:
>>                  a = int(part)
>> -                result.append(a)
>> -        return [int(i) for i in list(set(result))]
>> +                result |= {a}
>> +        return list(result)
>>
>>      def getcpulist(self):
>>          """ return the list of cpus tracked """
>> --
>> 2.27.0
>>
>>
>
> I'm guessing that the reason for the "set" was to remove any potential
> duplicates. Duplicates are not normally a problem in rteval, but if you
> want to ensure you handle any user input correctly, it makes sense to do
> that. I think your code is correct, but I'm not sure if it buys us
> anything.
>

It doesn't really, it should be functionaly identical to the existing code
- it's just "drive-by cleanup" really :)

> John