Remove unrefined reflections in index

[graeme-winter]

If indexing works and refinement fails in multi-lattice case, can have experiment ids assigned which do not correspond to experiments in the output. This change simply removes the experiment ids where there is no refined experiment.

Fixes #1349

[graeme-winter]

Can add a test once dials/data#210 merged

Test can use

from dials.array_family import flex
from dxtbx.model.experiment_list import ExperimentList

refl = flex.reflection_table.from_file("indexed.refl")
expt = ExperimentList.from_file("indexed.expt")

assert max(refl["id"]) + 1 == len(expt)

to ensure all indexed reflections have experiments

[codecov]

Codecov Report

Merging #1350 into master will increase coverage by 0.09%.
The diff coverage is 44.44%.

@@            Coverage Diff             @@
##           master    #1350      +/-   ##
==========================================
+ Coverage   64.22%   64.31%   +0.09%     
==========================================
  Files         616      616              
  Lines       69811    69833      +22     
  Branches     9536     9542       +6     
==========================================
+ Hits        44834    44913      +79     
+ Misses      23211    23135      -76     
- Partials     1766     1785      +19     

Impacted Files	Coverage Δ
algorithms/indexing/indexer.py	68.14% <44.44%> (-0.66%)	⬇️
algorithms/integration/report.py	86.59% <0.00%> (-0.69%)	⬇️
algorithms/symmetry/cosym/__init__.py	87.43% <0.00%> (+0.51%)	⬆️
test/test_plot_reflections.py	54.54% <0.00%> (+10.10%)	⬆️
command_line/plot_reflections.py	56.92% <0.00%> (+56.92%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a22751e...5200422. Read the comment docs.

[graeme-winter]

Added test which depends on dials_data PR

[graeme-winter]

@rjgildea test data added so will welcome your review

[rjgildea]

Looks to be a useful corner case to catch, however I think the code to reset the reflection indexed status should probably be better part of the try/except where the failed refinement was actually trapped, i.e. right after the failed experiment was deleted:

dials/algorithms/indexing/indexer.py

Lines 641 to 648 in e7e5292

	except (DialsRefineConfigError, DialsRefineRuntimeError) as e:
	if len(experiments) == 1:
	raise DialsIndexRefineError(str(e))
	had_refinement_error = True
	logger.info("Refinement failed:")
	logger.info(e)
	del experiments[-1]
	break

Is there any reason to put the new test in a separate file to the rest of the existing high-level indexing tests?

https://github.com/dials/dials/blob/master/algorithms/indexing/test_index.py

[graeme-winter]

Is there any reason to put the new test in a separate file to the rest of the existing high-level indexing tests?

https://github.com/dials/dials/blob/master/algorithms/indexing/test_index.py

This is a test of the command-line program so I put the tests where I would expect the test go to... which I think is reasonable.

[graeme-winter]

@rjgildea changes made according to your suggestions

[graeme-winter]

Is there any reason to put the new test in a separate file to the rest of the existing high-level indexing tests?
https://github.com/dials/dials/blob/master/algorithms/indexing/test_index.py

This is a test of the command-line program so I put the tests where I would expect the test go to... which I think is reasonable.

mmk well moved over - we can revisit this more widely I guess but this is consistent with the code base as it stands.

[graeme-winter]

Rerunning all tests before squash merging.

[graeme-winter]

@ndevenish will leave this one to rest for a bit on master but will want to pick across to release branch I think (not so sure about backport)

[ndevenish]

will want to pick across to release branch I think (not so sure about backport)

That is backporting

[graeme-winter]

will want to pick across to release branch I think (not so sure about backport)

That is backporting

Sorry, I meant back to 2.2 series 🙂

[Anthchirp]

This was merged with failing tests.
The tests did not lie.
Tests are now failing on master.
This should not have been merged.