Fix some minor issues in the indexing code

[benjaminhwilliams]

Catch some undefined variables and stuff like that.

• Previously entry_point in getattr call as part of strategy_class call in dials.algorithms.indexing.lattice_search.LatticeSearch.__init__ would be the value of the last entry point in pkg_resources.iter_entry_points("dials.index.lattice_search"), not the matching entry point.
• Buried deep in dials.algorithms.indexing.indexer.Indexer.index, there's a code path with an undefined variable refined_reflections. Presumably, reflections_for_refinement is what was intended.
• Remove the impossible default parameter params=None in initialising dials.algorithms.indexing.indexer.Indexer and its subclasses. None.indexing is always going to raise an AttributeError.

[benjaminhwilliams]

I find it a little difficult to figure out how to use these classes and what to pass them as arguments/how to mock those things, so I'm a bit stumped when it comes to writing tests.

Reverted. Thanks for untangling that for me.

[ndevenish]

Could we maybe add

refined_experiments = None
refined_reflections = None

before the main loop that this happens in e.g. at line 573:

dials/algorithms/indexing/indexer.py

Lines 573 to 574 in b36efe5

	for i_cycle in range(self.params.refinement_protocol.n_macro_cycles):

- that'd at least make it explicit that these variables are supposed to scope over multiple loop runs. I'm guessing the first time through the loop that self.params.refinement_protocol.mode == "repredict_only"?

[benjaminhwilliams]

Yes please, I'd be very happy with that, if @rjgildea is.

[ndevenish]

The strategy_class stuff in LatticeSearch looks like an actual bug, to me, unless someone can persuade me otherwise. In which case, it'll also need a .bugfix newsfragment 😀

[benjaminhwilliams]

Sure thing. I have a newsfragment that I haven't committed yet. I was holding off until we knew whether it ought to be .bugfix or .misc.

[codecov]

Codecov Report

Merging #1515 (b32eafc) into master (b36efe5) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1515      +/-   ##
==========================================
- Coverage   65.62%   65.62%   -0.01%     
==========================================
  Files         614      614              
  Lines       68965    68966       +1     
  Branches     9529     9528       -1     
==========================================
  Hits        45257    45257              
- Misses      21866    21867       +1     
  Partials     1842     1842              

[rjgildea]

The strategy_class stuff in LatticeSearch looks like an actual bug, to me, unless someone can persuade me otherwise. In which case, it'll also need a .bugfix newsfragment 😀

It hasn't been an issue thus far because there is currently only one entry point defined for dials.index.lattice_search so the loop implicitly exits after the first iteration meaning that it works by happy accident. Of course it would likely be broken if any further entry points were added.

dials/libtbx_refresh.py

Lines 23 to 25 in b36efe5

	"dials.index.lattice_search": [
	"low_res_spot_match = dials.algorithms.indexing.lattice_search:LowResSpotMatch"
	],

[ndevenish]

Ah 👍 I was thinking of basis vectors searches!

[rjgildea]

This change is actually in the wrong loop level, and breaks the very test that was designed to exercise the relevant try/except loop:

$ pytest algorithms/indexing/test_index.py::test_refinement_failure_on_max_lattices_a15 --regression
...
Traceback (most recent call last):
  File "/Users/rjgildea/software/cctbx_py3/build/../../cctbx/modules/dials/command_line/index.py", line 261, in <module>
    run()
  File "/Users/rjgildea/software/cctbx_py3/conda_base/python.app/Contents/lib/python3.8/contextlib.py", line 75, in inner
    return func(*args, **kwds)
  File "/Users/rjgildea/software/cctbx_py3/build/../../cctbx/modules/dials/command_line/index.py", line 244, in run
    indexed_experiments, indexed_reflections = index(
  File "/Users/rjgildea/software/cctbx_py3/build/../../cctbx/modules/dials/command_line/index.py", line 162, in index
    indexed_experiments, indexed_reflections = _index_experiments(
  File "/Users/rjgildea/software/cctbx_py3/build/../../cctbx/modules/dials/command_line/index.py", line 114, in _index_experiments
    idxr.index()
  File "/Users/rjgildea/software/cctbx/modules/dials/algorithms/indexing/indexer.py", line 659, in index
    sel = refined_reflections["id"] == last
TypeError: Please report this error to dials-support@lists.sourceforge.net: 'NoneType' object is not subscriptable

dials/algorithms/indexing/test_index.py

Lines 567 to 604 in fcfd7b4

	def test_refinement_failure_on_max_lattices_a15(dials_regression, tmpdir):
	"""Problem: Sometimes there is enough data to index, but not enough to
	refine. If this happens in the (N>1)th crystal of max_lattices, then
	all existing solutions are also dropped."""
	data_dir = os.path.join(dials_regression, "indexing_test_data", "lattice_failures")
	result = procrunner.run(
	[
	"dials.index",
	os.path.join(data_dir, "lpe4-2-a15_strong.pickle"),
	os.path.join(data_dir, "lpe4-2-a15_datablock.json"),
	"max_lattices=3",
	],
	working_directory=tmpdir,
	)
	assert not result.returncode and not result.stderr
	assert tmpdir.join("indexed.refl").check() and tmpdir.join("indexed.expt").check()
	experiments_list = load.experiment_list(
	tmpdir.join("indexed.expt").strpath, check_format=False
	)
	assert len(experiments_list) == 2
	# now try to reindex with existing model
	result = procrunner.run(
	[
	"dials.index",
	tmpdir.join("indexed.expt").strpath,
	os.path.join(data_dir, "lpe4-2-a15_strong.pickle"),
	"max_lattices=2",
	],
	working_directory=tmpdir,
	)
	assert not result.returncode and not result.stderr
	assert tmpdir.join("indexed.refl").check() and tmpdir.join("indexed.expt").check()
	experiments_list = load.experiment_list(
	tmpdir.join("indexed.expt").strpath, check_format=False
	)
	assert len(experiments_list) == 2