@bertiewooster I would suggest replacing GetUniqueCombinations() with GetUniqueCombinations_new() and then adding:

GetUniqueCombinations_new = GetUniqueCombinations

to maintain backwards compatibility

I applied the tests from GetUniqueCombinations() to GetUniqueCombinations_new() and one test is failing with error Combinations are not in the same order on about half of the platforms. Is it in fact a problem if the combinations come in the wrong order? In general I don't think of combinations as being ordered, but there might be a need for that here. If the order matters, I'll have to investigate further.

Here's the platforms the test fails on:

• Ubuntu_x64
• Ubuntu_x64_limitexternaldependencies
• Ubuntu_x64_py311
• Windows_VS2022_x64
• macOS_x64

whereas all tests succeed on the other platforms:

• Ubuntu_x64_cartridge
• Ubuntu_x64_java
• Windows_VS2022_x64_dll
• Windows_VS2022_x64_swig
• macOS_x64_java

This makes me wonder if itertools.product gives a different order for choice on some platforms. (The other conceivable possibility is that sorted gives different orders on on some platforms, but I would very much like to think that Python's tests would ensure that's not the case.)

It turns out that GetUniqueCombinations() and GetUniqueCombinations_new() return the same combinations but in different orders, for example:

old[0] = [(1, (11,)), (1, (12,)), (2, (31,))]
new[0] = [(1, (11,)), (1, (12,)), (2, (31,))]
^--same
old[1] = [(1, (11,)), (1, (12,)), (2, (32,))]
new[1] = [(1, (11,)), (1, (12,)), (2, (32,))]
^--same
old[2] = [(1, (11,)), (1, (13,)), (2, (31,))]
new[2] = [(1, (11,)), (1, (13,)), (2, (31,))]
^--same
old[3] = [(1, (11,)), (1, (13,)), (2, (32,))]
new[3] = [(1, (11,)), (1, (13,)), (2, (32,))]
^--same
old[4] = [(1, (12,)), (1, (13,)), (2, (31,))]
new[4] = [(1, (11,)), (1, (14,)), (2, (31,))]
^--different but old[4] is the same as new[6]
old[5] = [(1, (12,)), (1, (13,)), (2, (32,))]
new[5] = [(1, (11,)), (1, (14,)), (2, (32,))]
^--different but old[5] is the same as new[7]
old[6] = [(1, (11,)), (1, (14,)), (2, (31,))]
new[6] = [(1, (12,)), (1, (13,)), (2, (31,))]
^--different but old[6] is the same as new[4]
old[7] = [(1, (11,)), (1, (14,)), (2, (32,))]
new[7] = [(1, (12,)), (1, (13,)), (2, (32,))]
^--different but old[7] is the same as new[5]
old[8] = [(1, (12,)), (1, (14,)), (2, (31,))]
new[8] = [(1, (12,)), (1, (14,)), (2, (31,))]
^--same
old[9] = [(1, (12,)), (1, (14,)), (2, (32,))]
new[9] = [(1, (12,)), (1, (14,)), (2, (32,))]
^--same
old[10] = [(1, (13,)), (1, (14,)), (2, (31,))]
new[10] = [(1, (13,)), (1, (14,)), (2, (31,))]
^--same
old[11] = [(1, (13,)), (1, (14,)), (2, (32,))]
new[11] = [(1, (13,)), (1, (14,)), (2, (32,))]
^--same

GetUniqueCombinations_new() has the combinations in numerical, whereas GetUniqueCombinations() has some out of order. So unless there's some reason to keep the combinations out of numerical order, I suggest we re-order the expected results of the tests to put them in numerical order. Alternatively, we could relax the test to check that the expected and actual results contain the same combinations, but not assert that their order is the same.
(Edited to strike out first suggestion; relaxing the test to check that the expected and actual results contain the same combinations, but not assert that their order is the same, should make the tests pass even if there are differences in the order of combinations on platforms.)

@bertiewooster I would suggest replacing GetUniqueCombinations() with GetUniqueCombinations_new() and then adding:

GetUniqueCombinations_new = GetUniqueCombinations

to maintain backwards compatibility

@greglandrum is this what you mean, to redefine GetUniqueCombinations to use the new code, and then "alias" GetUniqueCombinations_new to GetUniqueCombinations in rdkit/Chem/Pharm2D/Utils.py?

def GetUniqueCombinations(choices, classes):
  """  Does the combinatorial explosion of the possible combinations
    of the elements of _choices_.

    """
  assert len(choices) == len(classes)
  combos = set()
  for choice in itertools.product(*choices):
    # If a choice occurs in more than one of the fields, we ignore this case
    if len(set(choice)) != len(choice):
      continue
    combos.add(tuple(sorted((cls, ch) for cls, ch in zip(classes, choice))))
  return [list(combo) for combo in sorted(combos)]

GetUniqueCombinations_new = GetUniqueCombinations

And do you recommend calling GetUniqueCombinations_new or GetUniqueCombinations in places where the code is called, e.g. in Pharm2D and in tests?

@greglandrum is this what you mean, to redefine GetUniqueCombinations to use the new code, and then "alias" GetUniqueCombinations_new to GetUniqueCombinations in rdkit/Chem/Pharm2D/Utils.py?

Yes.

And do you recommend calling GetUniqueCombinations_new or GetUniqueCombinations in places where the code is called,

I'd call GetUniqueCombinations() there.

Also, since the order of results is going to change, we should add a backwards incompatibility note in ReleaseNotes.md

Only test failure is JavaDiversityPickerTests on Ubuntu_x64_java, which seems unrelated to this PR.

Only test failure is JavaDiversityPickerTests on Ubuntu_x64_java, which seems unrelated to this PR.

yeah, that's an intermittent failure that happens... no need for concern