Best way to make a weighted sum

[Jorge Godoy]

Steven Taschuk <staschuk at telusplanet.net> writes:

> I don't know why you're using a class for this.  A simple function
> which calculates (and/or verifies) the checksum seems more natural.

The class has several other things. As I said, this was a snippet of
code. It showed you the __init__ and the other methods I was
interested in discussing.

Your approach made me think again on some other implementations I had
in mind before.

> And, despite the class name, you don't actually do a mod 11.  I
> assume this is a bug.

You haven't seen the rest of the class. This is not a bug.

The line that does the mod 11 is this one, in case you want to see it:


        self.modulo = self.soma % 11


Note that self.soma had other operations before being submitted
here. Some of our States do really weird things before the modulus
calculation. And some do these weird stuff after that.

>>     def __init__(self, ie=None, uf=None):
>>         
>>         self.estados = ['CE', 'AP', 'RJ'] 
>>         self.ie = ie
>>         self.soma = 0
>
> Why isn't the uf argument used?

I cut it out with the portuguese comments by mistake. Sorry.

      self.uf = uf

It is used later. 

> If uf is not in ['CE', 'AP', 'RJ'], then self.pesos simply does not
> get set; this will cause CalculaSoma to fail later, in a mysterious
> way.  Better to have an informative exception raised at this point,
> surely.

It will have all valid States. I wouldn't clutter the message with 27
States information and lots of different weights just to check an
approach :-) You could send me a better code without it :-)

Also there will be no exception since this is also bound to a domain
at the database server.

> It's also more natural imho to have the weights in a dict keyed by
> uf.  That dict can replace your self.estados, which I note is not
> otherwise used.

It is used later, by other methods that were not part of this part of
the code that I was interested in. But thanks for you comments. But,
on the other hand, using this dict keyed by uf approach seems
interesting. Makes the code cleaner.

> Also, the weights are conceptually a sequence, not a map; why not
> make them a list?

They are bound to the number position. Just that. A list would do fine
here. There's one state where they are: 1, 3, 4, 5, 6, 7, 8, 10. And
there are other variants of it too. You saw one in my original code,
where you asked if it was right starting with 2 and the going from 7
to 2 again... For some States I have to do that only once, for other
there are different rules for different categories of the
company... It's a mess.

>>     def CalculaSoma(self):
>> 
>>         ie = self.ie
>>         iter = len(ie)
>> 
>>         for i in ie:
>>             produto = i * self.pesos[iter]
>>             self.soma = self.soma + produto
>>             iter = iter - 1
>
> Why not return the sum?  See above.
>
> It's also not good form to use 'iter' as a variable name, since
> 'iter' is the name of a built-in function.

Oops. The full name is 'iteracao'. I changed it so that it would be
easier for you to understand what's it used for. I should have sent
the program without removing comments and without changing this name. 

I'm sorry for changing the code I posted. 

>> What I want is to sum the products of a certain 'ie' number where each
>> of its digits is multiplied by a certain weigth, depending on its
>> position inside the number. (This is for some documents verification
>> and 'ie' can vary in the amount of digits as well as its weigth can
>> vary from one place to another --- I know this is a mess, but it's the
>> way some Brazilian states did it...)
>
> An alternative:
>
> 	import operator
>
> 	weights = {}
> 	weights['CE'] = weights['AP'] = [9, 8, 7, 6, 5, 4, 3, 2]
> 	weights['RJ'] = [2, 7, 6, 5, 4, 3, 2] # is this right?

(weird isn't it? I also thought it was wrong but that's what's stated
at Rio de Janeiro's Treasury Secretary website:
http://www.sef.rj.gov.br/sub_adj_trib/sucief/calc_ie/index.shtml) 

> 	def checksum(ie, state):
> 		w = weights[state][-len(ie):]
> 		return reduce(operator.add, [x*y for x, y in zip(ie, w)]) % 11
>
> Imho this is much simpler, more straightforward, and easier to use.

Yes, it is. I didn't know zip(). It indeed facilitates a lot. I just
have to think, now, on how to add the several modifiers to this
checksum :-) It will be easy if there's no new weirdness here... 

The list approach also is interesting. I just have to think on how it
adapts to the cases where there are more possibilities in the same
State. 


You enlightened me a lot on this message, thanks.

-- 
Godoy.      <godoy at metalab.unc.edu>

There is a green, multi-legged creature crawling on your shoulder.