Reading the code of Point __init__ method I’ve found several inconsistencies and
issues. Some of them:
• When Point is build from ‘coordict’:
• it’s possible to provide coord value as a list, later this list is
silently truncated to the first element only, without type checking the value
• it’s possible to provide coord value as a array, later this array is
silently truncated to the first element only. In the case when length of
array is equal to 1, __init__ fails with mysterious IndexError. So this
code snippet works:
but this doesn’t
• this check
assert not isinstance(coorddict[c], (list, tuple)), 'type mismatch'
is useless as all possible values in coorddict are converted to array
during coorddict building
• if ‘coordtype’ argument is not provided, then self.coordtype is set to
‘float’ and is not equal self.coordarray.dtype.type (which is float64)
• self.coordarray and self.coordnames are built in different ways from
‘coorddict’ and ‘coordarray’/’coordnames’ pair but nevertheless there a lot of
duplicates in code (for example, type conversion and rank checking)
I’ve made the testsuite for Point and refactored a bit its __init__ method.
New version of Point __init__ method:
• uses the only way for building Point attributes
• warns user when provided list/array value for coord is truncated
• uses 'float' as default value for coordtype. To set it to ‘int’ user should
explicitly set ‘coordtype’ argument or pass array with int values
• has, I hope, cleaner and simpler code with less nested structure
I've submitted the patch on SourceForge:
Any feedback is appreciated.
I use 'nose' to run tests:
cd tests && nosetests point.py
Try it before patching Points.py to see failures fixed by patch.
PS. I would like to get involved in PyDSTool development. Let me know if you are interested in it.
Thanks for your observations, they are helpful. I will look over your patch soon and get back to you. I am interested in dev help, so please email me privately (e.g. via the SF project's mail link or my home web site) and we can discuss it further.
Log in to post a comment.