Retire cmp() function

[Anthchirp]

I rewrote all places where cmp is used and removed the cmp import.

In the interest of maximum compatibility I did not remove the declarations of the following functions that are now no longer needed, but are declared in public namespaces:

cctbx_project/cctbx/euclidean_model_matching.py

Lines 251 to 252 in 62adfcc

	def pair_sort_function(pair_a, pair_b):
	# Deprecated. Do not use

cctbx_project/cctbx/euclidean_model_matching.py

Lines 465 to 466 in 62adfcc

	def match_sort_function(match_a, match_b):
	# Deprecated. Do not use

cctbx_project/cctbx/sgtbx/__init__.py

Lines 832 to 833 in 62adfcc

	def compare_cb_op_as_hkl(a, b):
	# Deprecated. Do not use.

cctbx_project/cctbx/sgtbx/direct_space_asu/check_redundancies.py

Lines 237 to 238 in 62adfcc

	def compare_redundancies(a, b):
	# Deprecated. Do not use

Background:

The cmp function is gone. Importing it from past (which is part of the future package) is causing deprecation warnings all over the place due to PythonCharmers/python-future#551. The imp module will be removed in Python 3.10.

This also -as far as I can tell- would allow retiring the dependency on the future package. This is a good thing as the future package is a bit special.

Closes #493

[Anthchirp]

If there are no concerns I'll merge this on the coming weekend

[nksauter]

Markus, We'd prefer that you not touch files that were created at LBNL. Now that you've raised the issue we are studying it and will check in a CCTBX-centric solution to the problem. We do not believe your proposed changes preserve function, but we need more time to really investigate and sort things out. Nick Nicholas K. Sauter, Ph. D. Senior Scientist, Molecular Biophysics & Integrated Bioimaging Division Lawrence Berkeley National Laboratory 1 Cyclotron Rd., Bldg. 33R0345 Berkeley, CA 94720 (510) 486-5713

…

On Wed, Jul 15, 2020 at 8:45 AM Markus Gerstel ***@***.***> wrote: If there are no concerns I'll merge this on the coming weekend — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#498 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADQ24VT4KEWQOPMYY6GZQWLR3XFJ3ANCNFSM4OXHKKDQ> .

[Anthchirp]

Dear Nick,

good to hear from you. I have raised this issue previously (#493) however as I haven't had any response I went ahead and prepared this patchset, as suggested by our official CCTBX contribution guidelines, https://github.com/cctbx/cctbx_project/blob/master/CONTRIBUTING.md.

In fact, you will find I followed those to the letter. I followed local code styles and did not touch anything unrelated to the issue at hand. I explicitly preserved even the (most certainly unused) leftover helper functions, as pointed out in the pull request, to avoid any interface breakages.

All 95 test suites were successfully run on the pull request - as indicated by the 95 green ticks. So please forgive me when I am a bit confused about where you see any potential issues.
Would you be able to narrow down your concerns? Then I could move changes to any modules that you are concerned about into separate pull requests. This is easy enough - as you can see I intentionally laid out the pull request to have a single commit per module.

Obviously I'm happy for you to review and validate my changes and await your constructive and timely feedback. I would of course also gladly review any alternative pull requests.

Best wishes

-Markus

[bkpoon]

I'll have more time next week to look through this, but here are a few points based on a cursory look.

1. There is code duplication where the cmp function is implemented several times (if/else blocks that return -1, 0, or 1). Please put the standard implementation of cmp that does not use if/else in libtbx/math_utils.py. The round2 function is there, so might as well put cmp in there too.
2. Currently, the mmtbx tests are not being run on Azure Pipelines, so they have to be run manually. I'll run them in a phenix build. @olegsobolev can you take a look as well?
3. Some of the lambda functions can be replaced with itemgetter or attrgetter from the operator module. This was already done for some other sorting calls that used lambda functions.

[Anthchirp]

I've made the requested changes.

It would be great to get this in because the deprecation warnings are making it difficult to spot real issues:

[phyy-nx]

Rebasing onto master should fix the XFEL CI tests.

[phyy-nx]

Ah, I see the XFEL CI tests are run twice, once on the branch which is behind master, and once on the merged code (near the bottom of the list) where they are passing. No worries then.

[bkpoon]

There are still a few duplicate cmp implementations.

[bkpoon]

Looks good, thanks!

[Anthchirp]

Thank you for the review!