Hi,
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:
Point({'x': [0.0]})
but this doesn’t
Point({'x': array([0.0])})
• 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
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.
-Rob
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hi,
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:
Point({'x': [0.0]})but this doesn’t
Point({'x': array([0.0])})• this check
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:
https://sourceforge.net/tracker/?func=detail&aid=3614176&group_id=140858&atid=747667
Any feedback is appreciated.
I use 'nose' to run tests:
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.
Regards,
Vladimir Zakharov
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.
-Rob