Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bio.Cluster not checking arguments under Python 3 #340

Closed
wants to merge 2 commits into from

Conversation

peterjc
Copy link
Member

@peterjc peterjc commented Jun 16, 2014

I found this while investigating what is probably a big-endian issue in the C code which checks the single-letter arguments (not fixed, see mailing list discussion):

> On powerpc and s390x:
>
> | ======================================================================
> | ERROR: test_clusterdistance (test_Cluster.TestCluster)
> | ----------------------------------------------------------------------
> | Traceback (most recent call last):
> |   File "/«BUILDDIR»/python-biopython-1.64+dfsg/.pybuild/pythonX.Y_3.4/build/Tests/test_Cluster.py", line 212, in test_clusterdistance
> |     method='a', transpose=0)
> | ValueError: method should be a single character
> |
> | ======================================================================
> | ERROR: test_kcluster (test_Cluster.TestCluster)
> | ----------------------------------------------------------------------
> | Traceback (most recent call last):
> |   File "/«BUILDDIR»/python-biopython-1.64+dfsg/.pybuild/pythonX.Y_3.4/build/Tests/test_Cluster.py", line 141, in test_kcluster
> |     method='a', dist='e')
> | ValueError: method should be a single character
> |
> | ======================================================================
> | ERROR: test_somcluster (test_Cluster.TestCluster)
> | ----------------------------------------------------------------------
> | Traceback (most recent call last):
> |   File "/«BUILDDIR»/python-biopython-1.64+dfsg/.pybuild/pythonX.Y_3.4/build/Tests/test_Cluster.py", line 557, in test_somcluster
> |     inittau=0.02, niter=100, dist='e')
> | ValueError: distance should be a single character
> |
> | ======================================================================
> | ERROR: test_treecluster (test_Cluster.TestCluster)
> | ----------------------------------------------------------------------
> | Traceback (most recent call last):
> |   File "/«BUILDDIR»/python-biopython-1.64+dfsg/.pybuild/pythonX.Y_3.4/build/Tests/test_Cluster.py", line 290, in test_treecluster
> |     transpose=0, method='a', dist='e')
> | ValueError: method should be a single character
> |
> | ----------------------------------------------------------------------
> | Ran 210 tests in 337.138 seconds
> |
> | FAILED (failures = 1)
> | Skipping any tests requiring internet access
> | Python version: 3.4.1 (default, Jun  7 2014, 17:37:50)

Relevant C code:

 if (PyUnicode_Check(object))
     data = PyUnicode_AS_DATA(object);
 # ...
 if (strlen(data)!=1)
 { PyErr_Format(PyExc_ValueError, "method should be a single character (not '%s')", data);
   return 0;
 }

Jakob worte: But PyUnicode_AS_DATA() returns a pointer to UTF-32 data. On big-endian architectures (such as powerpc or s390x), the first byte is always 0, so strlen() returns 0.

@peterjc
Copy link
Member Author

peterjc commented Jun 18, 2014

Is the new unit test helpful though? i.e. 5b99854

@peterjc
Copy link
Member Author

peterjc commented Aug 4, 2014

@mdehoon How should the C code be updated?

@mdehoon
Copy link
Contributor

mdehoon commented Aug 15, 2014

I think this will not be super complicated. Just somebody needs to figure out how to get the length of a unicode string in C. Unfortunately I don't have the time now to go after this.

@peterjc
Copy link
Member Author

peterjc commented Aug 15, 2014

@mdehoon How does this look (on a test branch): peterjc@6741204

The (new) test passes: https://travis-ci.org/peterjc/biopython/builds/32624110

peterjc added a commit that referenced this pull request Aug 30, 2014
See previous commit and also Debian issue https://bugs.debian.org/751277
where this part of the C code was failing on Python 3 under PowerPC,
dicussion also CC'd to the Biopython mailing list.

This closes GitHub issue #340.
@peterjc
Copy link
Member Author

peterjc commented Aug 30, 2014

Fixed by 2a140f4

Tests added in c7811df

Thanks all :)

@peterjc peterjc closed this Aug 30, 2014
@peterjc peterjc deleted the cluster_single_char branch August 30, 2014 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants