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

Spot-count histogram: use math.ceil instead of round #1660

Merged
merged 5 commits into from
Apr 19, 2021
Merged

Conversation

rjgildea
Copy link
Contributor

Python uses a rounding half to even strategy, which means that 1.5 and 2.5 both round to 2, which resulted inconsistency in the spot-count depending on whether there was an even or odd number of images.

Python uses a rounding half to even strategy, which means that
1.5 and 2.5 both round to 2, which resulted inconsistency in the
spot-count depending on whether there was an even or odd number
of images.
@graeme-winter
Copy link
Contributor

🧐 It was that easy?

I spent a while looking at this in the past, was evidently looking in ${WRONG-PLACE} 🤬

Copy link
Contributor

@graeme-winter graeme-winter 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 fixing this - it was one of the more annoying "features" and I am genuinely shocked that the fix was this simple, but it presumably was so that's great!

@@ -95,3 +95,32 @@ def test_spot_counts_per_image_plot(dials_regression):
1 image 10"""
output = "\n".join(line.rstrip() for line in output.splitlines())
assert output == expected_output

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth having this test on e.g. 1...9 images of centroid test data to make sure that it works right for all the cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bug here was that the behaviour was inconsistent between even- and odd-length imagesets. We already had a test for output of even-length imagesets (1..10 above). I have added a test for an odd-length imageset below (1..3). We can change that to 1..9 if you think that would be better, but I don't think it would make any difference to the outcome.

@rjgildea
Copy link
Contributor Author

You can see the old behaviour by doing:

$ dials.import $(dials.data get -q x4wide)/X4_wide_M1S4_2_*.cbf

$ dials.find_spots imported.expt scan_range=1,2
...
Histogram of per-image spot count for imageset 0:
738 spots found on 2 images (max 396 / bin)
* 
**
**
**
**
**
**
**
**
**
12

$ dials.find_spots imported.expt scan_range=1,3
...
Histogram of per-image spot count for imageset 0:
958 spots found on 2 images (max 514 / bin)
* 
**
**
**
**
**
**
**
**
**
12

$ dials.find_spots imported.expt scan_range=1,4
...
Histogram of per-image spot count for imageset 0:
1190 spots found on 4 images (max 433 / bin)
*  *
*  *
*  *
*  *
*  *
*  *
** *
****
****
****
1  4

$ dials.find_spots imported.expt scan_range=1,5
...
Histogram of per-image spot count for imageset 0:
1449 spots found on 4 images (max 515 / bin)
*  *
*  *
*  *
*  *
*  *
*  *
****
****
****
****
1  4

With the changes in this PR, the behaviour is now as expected.

@codecov
Copy link

codecov bot commented Apr 16, 2021

Codecov Report

Merging #1660 (5c6750f) into main (e5213f4) will decrease coverage by 0.00%.
The diff coverage is 42.85%.

❗ Current head 5c6750f differs from pull request most recent head fdb5067. Consider uploading reports for the commit fdb5067 to get more accurate results

@@            Coverage Diff             @@
##             main    #1660      +/-   ##
==========================================
- Coverage   66.70%   66.69%   -0.01%     
==========================================
  Files         616      616              
  Lines       69057    69062       +5     
  Branches     9606     9607       +1     
==========================================
- Hits        46063    46062       -1     
- Misses      21060    21065       +5     
- Partials     1934     1935       +1     

@rjgildea rjgildea merged commit a66ee17 into main Apr 19, 2021
@rjgildea rjgildea deleted the ascii-art-ceil branch April 19, 2021 09:01
rjgildea added a commit that referenced this pull request Apr 19, 2021
Python uses a rounding half to even strategy, which means that
1.5 and 2.5 both round to 2, which resulted inconsistency in the
spot-count depending on whether there was an even or odd number
of images.
DiamondLightSource-build-server added a commit that referenced this pull request Apr 19, 2021
Bugfixes
--------

- ``dials.scale``: Fix crash when full-matrix minimisation is unsuccessful due to indeterminate normal equations. (#1653)
- ``dials.scale``: Fix crash when no reflections remain after initial filtering. (#1654)
- ``dials.export``: Fix error observed with ``format=mmcif`` for narrow sweeps with low symmetry (#1656)
- Fix image numbering inconsistency in ascii histogram of per-image spot counts (#1660)
DiamondLightSource-build-server added a commit that referenced this pull request Apr 19, 2021
Bugfixes
--------

- ``dials.scale``: Fix crash when full-matrix minimisation is unsuccessful due to indeterminate normal equations. (#1653)
- ``dials.scale``: Fix crash when no reflections remain after initial filtering. (#1654)
- ``dials.export``: Fix error observed with ``format=mmcif`` for narrow sweeps with low symmetry (#1656)
- Fix image numbering inconsistency in ascii histogram of per-image spot counts (#1660)
- ``dials.find_spots_server``: Significant performance improvement for HDF5 grid scans. (#1665)
DiamondLightSource-build-server added a commit that referenced this pull request Apr 20, 2021
Bugfixes
--------

- ``dials.scale``: Fix crash when full-matrix minimisation is unsuccessful due to indeterminate normal equations. (#1653)
- ``dials.scale``: Fix crash when no reflections remain after initial filtering. (#1654)
- ``dials.export``: Fix error observed with ``format=mmcif`` for narrow sweeps with low symmetry (#1656)
- Fix image numbering inconsistency in ascii histogram of per-image spot counts (#1660)
- ``dials.find_spots_server``: Significant performance improvement for HDF5 grid scans. (#1665)
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