SimTk/openmm — pull request #307 by swails (2014-01-28 13:28:08+00:00)
Title: Fix bug in _is_string

It took me a second to find out what was being done in openmm._is_string and why, but the except block also has to catch ValueError. In some of the code I wrote, I got the following error (only showing the end of the traceback):

File "/home/swails/amber/bin/ParmedTools/simulations/openmm.py", line 492, in simulate
simulation.context.setPositions(position_container.positions)
File "/usr/local/openmm/dev/openmm/python2.7/simtk/openmm/openmm.py", line 3715, in setPositions
try: args=stripUnits(args)
File "/usr/local/openmm/dev/openmm/python2.7/simtk/openmm/openmm.py", line 1167, in stripUnits
elif not _is_string(arg):
File "/usr/local/openmm/dev/openmm/python2.7/simtk/openmm/openmm.py", line 1104, in _is_string
if first_item == inner_item:
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

The problem here is that position_containers.positions is actually a NATOM-length tuple of Vec3 quantities. Therefore, both first_item and inner_item resolve to valid python objects: a Vec3() and the first value from that Vec3(), respectively. Attempting to compare the two results in the ValueError which passes through uncaught.

Perhaps _is_string should be replaced with just

# Python2 only; catches str, unicode, and bytes
isinstance(x, basestring)

# Python3
isinstance(x, str) or isinstance(x, bytes)

instead? I understand that the code is designed to catch any infinite containers, but I suspect that the existing code will only work on strings and string-likes since it assumes that the repeating part of the container-in-container comes first. (i.e., if it was designed to catch repeating decimals it would catch 1/3 but not 1/6)

As a side note, I was able to avoid the exception by making the positions variable a quantity-tuple of NATOM Vec3s (rather than a non-quantity tuple of NATOM Vec3 quantities). I haven't investigated enough to see why that fixes things, but with my proposed change either one works. (Making the whole tuple a quantity rather than each Vec3 is a better route, I think, but it would be nice to support both).