Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dials.cosym target fail bug #1993

Merged
merged 7 commits into from
Feb 21, 2022
Merged

Fix dials.cosym target fail bug #1993

merged 7 commits into from
Feb 21, 2022

Conversation

jbeilstenedmands
Copy link
Contributor

@jbeilstenedmands jbeilstenedmands commented Feb 2, 2022

In dials.cosym, I found a case where a low resolution cutoff in the resolution filter caused at least one full dataset to be removed, leading to a stacktrace during the minimisation at:

>       assert (x.size // self.dim) == (len(self._lattices) * len(self.sym_ops))
E       AssertionError: Please report this error to dials-support@lists.sourceforge.net:

/dials/modules/dials/algorithms/symmetry/cosym/target.py:271: AssertionError

Of course a suitable d_min could just be set, but I still thought it would be good to avoid crashing out and handle this more gracefully.

I have added a test to reproduce this, and the fix is fairly straightforward, just making sure to use the correct number of datasets in the procedure (which could be reduced compared to upon initialisation). In the example test, only 7/8 datasets remain after the resolution filter, so only 7 reindexing operators can be determined. The only outstanding question is whether the filtered-out dataset should remain in the output refl, expt files. As no reindexing operator can be determined, I don't believe they should, so this proposed changeset removes them as they may not be consistently indexed.

p.s. I updated all the tests to use pathlib objects

@codecov
Copy link

codecov bot commented Feb 2, 2022

Codecov Report

Merging #1993 (0824621) into main (92daf13) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1993      +/-   ##
==========================================
- Coverage   68.82%   68.82%   -0.01%     
==========================================
  Files         649      649              
  Lines       75294    75318      +24     
  Branches    10782    10787       +5     
==========================================
+ Hits        51824    51835      +11     
- Misses      21446    21458      +12     
- Partials     2024     2025       +1     

Copy link
Contributor

@rjgildea rjgildea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the bug fix and moving tests to use pathlib.Path. A few very minor comments about the pathlib usage, but otherwise looks good to me 👍

command_line/cosym.py Outdated Show resolved Hide resolved
tests/command_line/test_cosym.py Outdated Show resolved Hide resolved
tests/command_line/test_cosym.py Outdated Show resolved Hide resolved
tests/command_line/test_cosym.py Outdated Show resolved Hide resolved
tests/command_line/test_cosym.py Outdated Show resolved Hide resolved
tests/command_line/test_cosym.py Outdated Show resolved Hide resolved
tests/command_line/test_cosym.py Outdated Show resolved Hide resolved
tests/command_line/test_cosym.py Outdated Show resolved Hide resolved
tests/command_line/test_cosym.py Outdated Show resolved Hide resolved
command_line/cosym.py Outdated Show resolved Hide resolved
Co-authored-by: Richard Gildea <rjgildea@users.noreply.github.com>
@ndevenish ndevenish merged commit 4256b3e into main Feb 21, 2022
@ndevenish ndevenish deleted the cosym_target_fail_bug branch February 21, 2022 18:47
ndevenish pushed a commit that referenced this pull request Feb 21, 2022
In rare cases, a low resolution cutoff in the resolution filter caused
at least one full dataset to be removed, leading to a crash during
minimisation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants