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

Allow reflection_table.remove_on_experiment_identifiers when len==0 #2298

Merged
merged 3 commits into from
Dec 9, 2022

Conversation

Baharis
Copy link
Contributor

@Baharis Baharis commented Dec 6, 2022

I encountered what I believe is a bug in flex behavior, and since it stops my jobs, I decided to suggest a fix. This PR is tiny, but it is my first one here, so please let me know if I'm missing anything.

When filtering problematic dataset in cctbx.xfel, I encountered multiple instances of the following error:

File "[...]/cctbx_project/xfel/merging/application/filter/experiment_filter.py", line 50, in remove_experiments
  reflections.remove_on_experiment_identifiers(experiment_ids_to_remove)
File "[...]/dials/src/dials/array_family/flex_ext.py", line 1190, in remove_on_experiment_identifiers
  assert "id" in self

The dataset I'm processing has very few reflections, so the error most likely stems from the fact, that an empty flex.reflection_table() does not have key id. However, all methods utilized within remove_on_experiment_identifiers() can act on empty table. In my understanding, the assert "id" in self statement is supposed to catch tables which have reflections, but the reflections have no id.

This PR suggests to add a conditional statement before the assertion, so that the presence of id keyword is required only if the table has any reflections to begin with. As a result, the method should act as a null-operation for a zero-length table. Alternatively, the method could just return self if len(self)==0, but this would create two return points, which is generally undesirable.

@Baharis Baharis marked this pull request as ready for review December 6, 2022 20:38
@codecov
Copy link

codecov bot commented Dec 6, 2022

Codecov Report

Merging #2298 (529592f) into main (d99f27f) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #2298      +/-   ##
==========================================
- Coverage   80.55%   80.55%   -0.01%     
==========================================
  Files         586      586              
  Lines       66986    66987       +1     
  Branches     8917     8918       +1     
==========================================
- Hits        53963    53961       -2     
  Misses      10960    10960              
- Partials     2063     2066       +3     

@phyy-nx phyy-nx requested review from jbeilstenedmands and removed request for Anthchirp December 8, 2022 19:26
Copy link
Contributor

@jbeilstenedmands jbeilstenedmands left a comment

Choose a reason for hiding this comment

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

Thanks!

@phyy-nx phyy-nx merged commit 397be0c into main Dec 9, 2022
@phyy-nx phyy-nx deleted the flex_allow_removal_from_empty branch December 9, 2022 21:00
dagewa pushed a commit that referenced this pull request Feb 20, 2023
…0` (#2298)

Allow `reflection_table.remove_on_experiment_identifiers` when `len==0`
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

3 participants