Make some tools more robust to few reflections #1263
Conversation
There was a problem hiding this comment.
I note that this may be one case that would perhaps benefit from splitting up the changes into multiple commits (and not necessarily squash/merging), rather than grouping together several logically unrelated changes in one commit. This would make e.g. reverting the individual changes easier if it later turns out that one of them caused some issues.
|
@rjgildea, regarding keeping some degree of commit history rather than a full squash-merge, I agree. Once this PR gets approval, I'll force-push a picked/squashed version with different changes kept separate and ask for approval again. |
|
I'm afraid I've given up on the idea of unpicking every place where problems occur and building unit tests for each. Instead I've just built some simple integration tests. Let me know if you'd like me to revisit the unit tests, especially since it might avoid things like the previously undetected bug in |
|
Ran through a standard data set with this change set, made no difference to a The reflections coming out are identical 🙂 gives nothing => from that perspective safe |
|
Great, thanks. Would you be happy for me to merge? |
|
In the meantime, I'll work on restructuring the commits to be a little more minimal, then we can break with convention and merge without squashing, as recommended by @rjgildea, since there is value in being able to revert logically distinct components of this PR. |
Test the behaviour of dials.symmetry, dials.scale, dials.merge and dials.report for very small numbers of reflections.
benjaminhwilliams
force-pushed
the
few-reflections
branch
from
3a211b7
to
9f13473
Compare
|
@rjgildea indicated offline that he wanted to take a look before I merged... |
Test the behaviour of dials.symmetry, dials.scale, dials.merge and dials.report for very small numbers of reflections.
Simply return an null value, which is correctly interpreted downstream so that Xtriage plots aren't generated.
Do not raise an exception if Wilson stats cannot be calculated for printing. Instead, log an error message and continue. Also correct a typo in the help text of a PHIL parameter.
Otherwise, if step == 0, ma.setup_binner_d_star_sq_step fails with an AssertionError.
Previously, if the anomalous merging statistics could not be calculated, the non-anomalous merging statistics would not be calculated either, even if they could be. Make the merging statistics calculation more robust so that not calculating anomalous merging statistics does not break downstream reporting.
Otherwise, non-integer experiment IDs cause an exception. Also raise a ValueError if all experiments have been rejected, since we cannot possibly continue processing no data.
If reflection intensity normalisation cannot be performed (for example if there are too few reflections), then fall back on using un-normalised intensities and log a warning to that effect.
benjaminhwilliams
force-pushed
the
few-reflections
branch
from
9f13473
to
2e592e0
Compare
Some of the DIALS tools furthest downstream, such as
dials.scale,dials.symmetryanddials.merge, rely on receiving a sufficient number of unique reflections to perform their calculations. Existing checks that the input is valid are not explicit and are insufficient to catch all failure modes. Changes here attempt to rationalise some behaviour in cases of very few input reflections, as from a very narrow wedge of a rotation data set.Details of changes:
dials.algorithms.scaling.ScalingAlgorithm.remove_bad_dataresults in no remaining data, raise an exception.remove_bad_datato correctly handle experiment UID strings.dials.algorithms.scaling.Ih_table.IhTableBlock.setup_binner, be less prescriptive in the choice of binner parameters in the case of few reflections.modules.dials.algorithms.merging.merge.merge_and_truncatedoes not receive enough reflections to calculate Wilson statistics for log output, catch the resulting exception and log an error-level message instead.dials.algorithms.symmetry.symmetry_base._normalise, if an exception is encountered when trying to perform the normalisation, rather than raise a cryptic exception, simply log a warning and default to un-normalised intensities.modules.dials.command_line.merge.phil_scope.