dials.index: do not write out reflections for removed experiments #2653

dagewa · 2024-04-23T14:41:40Z

If a new crystal model was found to be too similar to an existing model, but only after some refinement was done, then indexed reflections associated with that experiment made it into the output, and crashed dials.integrate.

This PR fixes that corner case by resetting such reflections to unindexed straight after the experiment was removed.

…imilar to an existing model, any reflections associated with that experiment id should be reset to unindexed. Reuse existing code for removing reflections with a certain id by refactoring that into its own function.

jbeilstenedmands · 2024-04-23T14:49:41Z

src/dials/algorithms/indexing/indexer.py

+                    try:
+                        # Unset ids for the last experiment that is now deleted
+                        self._remove_id_from_reflections(len(experiments))
+                    except AttributeError:


What is the scenario in which the above call would raise an AttributeError?

I wondered if there is a circumstance in which self.refined_reflections does not exist because it's not actually defined until later. However I think the condition in this block is only true if there are already two crystals, in which case self.refined_reflections must have been created during the handling of the first one.

Maybe it would be better if self.refined_reflections was always created at the start of the index function, even as an empty reflection table.

In that case i think adding in the except AttributeError is misleading, and we should either define self.refined_reflections at the start or add a comment to explain that this is only called once self.refined_reflections exists.

codecov · 2024-04-23T16:01:55Z

Codecov Report

Attention: Patch coverage is 93.75000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 78.31%. Comparing base (3e0807b) to head (8859042).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2653   +/-   ##
=======================================
  Coverage   78.31%   78.31%           
=======================================
  Files         613      613           
  Lines       75504    75507    +3     
  Branches    10792    10792           
=======================================
+ Hits        59128    59131    +3     
  Misses      14190    14190           
  Partials     2186     2186

jbeilstenedmands · 2024-04-24T14:32:23Z

src/dials/algorithms/indexing/indexer.py

@@ -613,6 +613,11 @@ def index(self):

                if self._check_have_similar_crystal_models(experiments):
                    have_similar_crystal_models = True
+                    try:
+                        # Unset ids for the last experiment that is now deleted
+                        self._remove_id_from_reflections(len(experiments))


I think this may be overlooking the case of shared crystal models. A possibility is that you have four experiments and two crystals (e.g. multi-sweep indexing with two lattices). In that case you need to match experiments that share the crystal to be removed, which is what is done in the other case of _remove_id_from_reflections below.

I see. I think the additional shared crystal matching could be incorporated into the function so there's one place that does the removal properly. However, this is an area where I've run into trouble before (#2609). Maybe there are use-cases where we don't want to throw out all experiments if the crystal model is bad for just one of them. This might need more thought...

self.refined_reflections will exist if self._check_have_similar_crystal_models(experiments) is True

where we want to remove the last crystal. This function deletes the experiments with that crystal model, including any times it is shared, and also removes all associated reflections.

* Add dials.import test for multi-panel distance overrides.

New dials program to calculate correlation matrices between datasets as a stand-alone module extracting methods from xia2.multiplex. --------- Co-authored-by: James Beilsten-Edmands <30625594+jbeilstenedmands@users.noreply.github.com> Co-authored-by: Ben Williams <benjaminhwilliams@users.noreply.github.com>

…2658) * For a multi-axis goniometer we set the overall rotation axis by altering the orientation of the base axis. That way we are not changing the relationship between axes of the goniometer (so we still trust the header angles), we are just rotating the whole goniometer to fit the observations

* Fix dials.show for time of flight experiments.

* Trap d_min > d_max in masking Stop with an error if d_min > d_max as the user almost certainly did not mean this. If they are equal still no spots will be found but no error will be raised. Fixes #2663

…-rejected-experiment

dagewa · 2024-05-15T11:51:54Z

I attempted to include the deletion of shared crystal models when removing a crystal and reflections that are too similar to a previously-found crystal. However, this now causes a test failure in test_multi_lattice_multi_sweep_joint. This can also be investigated by running the command

dials.index $(dials.data get -q l_cysteine_dials_output)/indexed.* max_lattices=2

where it is seen that all experiments and reflections are removed. I have not been able to figure out why this is happening yet.

dagewa · 2024-05-15T12:31:52Z

Ah, I think I see the problem. I did not need to include the removal of the crystal models, because this is actually done in _check_have_similar_crystal_models (it does more than just check...)

… places" This reverts commit c445b96.

…al_models - Also remove the associated reflections here

dagewa · 2024-05-15T19:41:56Z

I think this addresses the comments and is ready for re-review

dagewa added 2 commits April 23, 2024 15:11

News

ed1ac67

dagewa linked an issue Apr 23, 2024 that may be closed by this pull request

dials.index writes out reflections for a rejected experiment #2652

Open

dagewa requested a review from jbeilstenedmands April 23, 2024 14:41

Rename newsfragments/xxx.bugfix to newsfragments/2653.bugfix

9f890b1

jbeilstenedmands reviewed Apr 23, 2024

View reviewed changes

jbeilstenedmands reviewed Apr 24, 2024

View reviewed changes

dagewa and others added 10 commits May 15, 2024 12:45

No need for try/except here

697481d

self.refined_reflections will exist if self._check_have_similar_crystal_models(experiments) is True

Use new function _remove_last_crystal_and_reflections in both places

c445b96

where we want to remove the last crystal. This function deletes the experiments with that crystal model, including any times it is shared, and also removes all associated reflections.

Test setting distance for multipanel detectors (#2616)

766a0e2

* Add dials.import test for multi-panel distance overrides.

Updates to dials.scale error modelling to handle stills data (#2654)

87dcc83

Fix dials.show for time of flight experiments. (#2660)

0a6c8bf

* Fix dials.show for time of flight experiments.

Add weighting option to cosym CC calculation (#2666)

b1e2882

Trap d_min > d_max in masking (#2664)

e22d2a7

* Trap d_min > d_max in masking Stop with an error if d_min > d_max as the user almost certainly did not mean this. If they are equal still no spots will be found but no error will be raised. Fixes #2663

Merge branch 'main' into 2652-dialsindex-writes-out-reflections-for-a…

f340104

…-rejected-experiment

dagewa added 3 commits May 15, 2024 13:37

Revert "Use new function _remove_last_crystal_and_reflections in both…

38f2107

… places" This reverts commit c445b96.

- Rename _check_have_similar_crystal_models --> _remove_similar_cryst…

a6fe102

…al_models - Also remove the associated reflections here

Also must rename call in stills_indexer

8859042

dagewa requested a review from jbeilstenedmands May 15, 2024 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dials.index: do not write out reflections for removed experiments #2653

dials.index: do not write out reflections for removed experiments #2653

dagewa commented Apr 23, 2024

jbeilstenedmands Apr 23, 2024

dagewa Apr 23, 2024 •

edited

jbeilstenedmands Apr 24, 2024

codecov bot commented Apr 23, 2024 •

edited

jbeilstenedmands Apr 24, 2024 •

edited

dagewa Apr 24, 2024

dagewa commented May 15, 2024

dagewa commented May 15, 2024

dagewa commented May 15, 2024

dials.index: do not write out reflections for removed experiments #2653

Are you sure you want to change the base?

dials.index: do not write out reflections for removed experiments #2653

Conversation

dagewa commented Apr 23, 2024

jbeilstenedmands Apr 23, 2024

Choose a reason for hiding this comment

dagewa Apr 23, 2024 • edited

Choose a reason for hiding this comment

jbeilstenedmands Apr 24, 2024

Choose a reason for hiding this comment

codecov bot commented Apr 23, 2024 • edited

Codecov Report

jbeilstenedmands Apr 24, 2024 • edited

Choose a reason for hiding this comment

dagewa Apr 24, 2024

Choose a reason for hiding this comment

dagewa commented May 15, 2024

dagewa commented May 15, 2024

dagewa commented May 15, 2024

dagewa Apr 23, 2024 •

edited

codecov bot commented Apr 23, 2024 •

edited

jbeilstenedmands Apr 24, 2024 •

edited