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

Import still ImageSequences as N experiments #1014

Merged
merged 20 commits into from
Dec 9, 2019

Conversation

graeme-winter
Copy link
Contributor

In parallel with

cctbx/dxtbx#117

Trying as @rjgildea suggested to do "the right thing" on import -

  • this starts in finite time (good)
  • it does iterate over experiments (bad)

@graeme-winter graeme-winter changed the title Hunting onions Import still ImageSequences as N experiments Nov 15, 2019
@graeme-winter
Copy link
Contributor Author

This change recreates the "find spots takes forever to start" feature :-/

@graeme-winter
Copy link
Contributor Author

With the changes in cctbx/dxtbx#118 now loads before the heat death of the universe which is a good thing. This PR includes code which is sufficient to find spots on a long experiment list against one underlying image set correctly and restores the ascii art output e.g.

Histogram of per-image spot count for imageset 237:
20473 spots found on 238 images (max 934 / bin)
*                                                           
*    * *    *  *   *                                        
**   ****   ** *   *  *   *                                 
**   ****   ****   ****   *  *                              
**   ****   ****   ****   ** *   *                          
**  *****   ****  *****  *** ** **   *                      
**  *****  *****  ****** *** ** **   *                      
**  *****  ****** ****** ****** **  **                      
*** ****** ****** ****** ****** **  ** **                   
************************************************************
1                         image                          238

There is a certain amount of boiler plate here which is duplicated and could probably be improved by pulling into a central location.

N.B. there is a large extant issue here: the image viewer canna cope with this as a concept - need to get it to understand that > 1 experiment can point at an image set. Had a look at this for an hour and failed.

@graeme-winter graeme-winter marked this pull request as ready for review November 18, 2019 10:38
@ndevenish
Copy link
Member

N.B. there is a large extant issue here: the image viewer canna cope with this as a concept - need to get it to understand that > 1 experiment can point at an image set. Had a look at this for an hour and failed.

Does this mean that currently image_viewer fails to open these files?

@graeme-winter
Copy link
Contributor Author

graeme-winter commented Nov 18, 2019 via email

@graeme-winter
Copy link
Contributor Author

Also:

Does not do the right thing on Pilatus grid scans:

Setting spotfinder.filter.min_spot_size=3
Configuring spot finder from input parameters
Traceback (most recent call last):
  File "/Users/graeme/svn/cctbx/build/../modules/dials/command_line/find_spots.py", line 200, in <module>
    script.run()
  File "/Users/graeme/svn/cctbx/build/../modules/dials/command_line/find_spots.py", line 128, in run
    reflections = flex.reflection_table.from_observations(experiments, params)
  File "/Users/graeme/svn/cctbx/modules/dials/array_family/flex.py", line 198, in from_observations
    return find_spots(experiments)
  File "/Users/graeme/svn/cctbx/modules/dials/algorithms/spot_finding/finder.py", line 760, in __call__
    imageset.set_scan(imageset_scan[imageset])
RuntimeError: Please report this error to dials-support@lists.sourceforge.net: dxtbx Internal Error: /Users/graeme/svn/cctbx/modules/dxtbx/imageset.h(367): DXTBX_ASSERT(index < scans_.size()) failure.

=> something else to investigate, yay.

@graeme-winter
Copy link
Contributor Author

Does the right thing on pilatus now => think we have something worth discussion now

Found 190 strong pixels on image 226
Found 387 strong pixels on image 227
Found 248 strong pixels on image 228
Found 228 strong pixels on image 229
Found 230 strong pixels on image 230

Extracted 5799 spots
Removed 2907 spots with size < 3 pixels
Removed 0 spots with size > 1000 pixels
Calculated 2892 spot centroids
Calculated 2892 spot intensities
Filtered 2578 of 2892 spots by peak-centroid distance

Histogram of per-image spot count for imageset 229:
2578 spots found on 230 images (max 482 / bin)
                                *                           
                          *     *                           
                      *   *     *                           
                      *   *     *                           
                      *   *     *      *                    
                     **   *     *      *                    
                     **   *     *      *                    
                     **   *     *      *                    
             *       **   *     **     *                    
             *       **  ***   ***     **   *               
1                         image                          230

--------------------------------------------------------------------------------
Saved 2578 reflections to strong.refl
Time Taken: 23.278084

@graeme-winter
Copy link
Contributor Author

Unsurprisingly a lot of tests now fail... usually those which have stills in name - e.g.

            # Fix up the experiment ID's now
            table["id"] = flex.int(table.nrows(), -1)
            for i, experiment in enumerate(experiments):
                if experiment.imageset is not imageset:
                    continue
>               z0, z1 = experiment.scan.get_array_range()
E               AttributeError: 'NoneType' object has no attribute 'get_array_range'

/dls/science/users/gw56/svn/cctbx/modules/dials/algorithms/spot_finding/finder.py:771: AttributeError

@graeme-winter
Copy link
Contributor Author

With last couple of hacks to dxtbx heat death branch getting full tests passing => review please 🙂

Copy link
Member

@dagewa dagewa left a comment

Choose a reason for hiding this comment

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

I think this needs some specific tests added before merging (but I approve of the concept)

@graeme-winter
Copy link
Contributor Author

Note to self: this explodes if you try to import frame NN to NN+j not starting at 1 (if files 1...NN-1 not on disk)

@graeme-winter
Copy link
Contributor Author

Note to self: this explodes if you try to import frame NN to NN+j not starting at 1 (if files 1...NN-1 not on disk)

Fixed in cctbx/dxtbx@77b8ba4

With proper use of batch_offset added in cctbx/dxtbx@77b8ba4 no longer need this workaround
And untangle the shell game while I am there
@graeme-winter graeme-winter mentioned this pull request Dec 3, 2019
@Anthchirp Anthchirp added this to the DIALS 2.1 milestone Dec 4, 2019
@graeme-winter
Copy link
Contributor Author

#info I would like to see this merged real soon

@dagewa
Copy link
Member

dagewa commented Dec 8, 2019

There's a request for some tests to be added, plus @jmp1985 made some comments that could be read as requesting changes. Once these points are resolved I think it is ready.

Useful as ovreriding the geometry was not previously csetting is_still
@graeme-winter
Copy link
Contributor Author

Added tests as requested - valid feedback thank you.

Using centroid test data and importing as still sequence highlighted issue in dxtbx, also fixed.

@graeme-winter
Copy link
Contributor Author

@dagewa added tests 🙂

@graeme-winter
Copy link
Contributor Author

👍 appreciate the feedback and help thank you - will squash merge in a mo

@graeme-winter graeme-winter merged commit dfd9bfc into master Dec 9, 2019
@graeme-winter graeme-winter deleted the imageset-experiment-iterator branch December 9, 2019 16:05
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

5 participants