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

Correct array/scan-point index mixup #1933

Merged
merged 5 commits into from
Jan 14, 2022
Merged

Correct array/scan-point index mixup #1933

merged 5 commits into from
Jan 14, 2022

Conversation

dagewa
Copy link
Member

@dagewa dagewa commented Nov 17, 2021

Correct instances where an array-index, which might not start from 0, is conflated with a scan-point index, which always starts from zero.

Two of these are in plot generation code. This issue is more pernicious than first thought (#1929) as it is not limited to merely shifting of the X-axis. If there are multiple crystals in the experiment, and these have different scan ranges (possible due to scan-trimming) then the plots themselves are also shifted relative to one another.

Here's an example with two crystals in the same imageset with scans that go from -24.1° to 24.9° and from -35.1° to 22.9°. Previously the orientation plot from dials.plot_scan_varying_model incorrectly showed both scan-varying models starting from -44.1°, which is the result of scan.get_angle_from_array_index(0) from either of the scans.

wrong behaviour on main:
orientation

WIth these changes the two scans are correctly displayed as starting at -24.1° and -35.1°.

correct behaviour on branch:
orientation

The third instance is in dials.sequence_to_stills, for which Aaron is tagged for review.

@dagewa dagewa requested a review from phyy-nx November 17, 2021 09:53
@dagewa
Copy link
Member Author

dagewa commented Nov 17, 2021

@phyy-nx I am working on some code in a different branch in which the scan-varying model at the start and end of a frame is interpolated to give a crystal model for the frame centre. I think that concept could be introduced in dials.sequence_to_stills too, which I can do outside this PR.

@phyy-nx
Copy link
Member

phyy-nx commented Nov 18, 2021

@kmdalton would you or someone in your group like to take a look at this? You use this more than I.

@kmdalton
Copy link
Contributor

@PrinceWalnut, can you look at these modifications to dials.sequence_to_stills and see if they impact your indexing code? (ideally pull this version and run it)

@PrinceWalnut
Copy link

PrinceWalnut commented Nov 18, 2021

@PrinceWalnut, can you look at these modifications to dials.sequence_to_stills and see if they impact your indexing code? (ideally pull this version and run it)

This version works as well as the previous version, which is to say it works for scan-varying crystals but not scan-static crystal models, or for models which have not had shoeboxes assigned yet (particularly those for which force2d=True was set during spotfinding). You can get around the shoebox issue by using the following snippet to select refls for including into the still:

_, _, _, _, z1, z2 = reflections["bbox"].parts()
subrefls = reflections.select((i_scan_point >= z1) & (i_scan_point < z2))

To handle scan-static models, the crystal.get_A_at_scan_point() function needs to either be dropped in favour of manually applying the appropriate rotation matrix to crystal.get_A() or by including that functionality into the function itself and detecting scan-static models to use it then.

EDIT: I have developed a few variants of this script specifically for dealing with scan-static and for multi-beam experiments. Perhaps these should be added as either separate scripts, or into this script with some smart selection depending on arguments?

@PrinceWalnut
Copy link

I would suggest approving this request, and I can update my fork and my scripts and form a pull request later with the extra functionality I have. My scripts will also have this array bug, so I need to change them first.

@PrinceWalnut
Copy link

@phyy-nx That's all the input I have here. I can pull this fix and add my own for the mentioned issues above if it's useful for you. Otherwise LGTM

@phyy-nx
Copy link
Member

phyy-nx commented Nov 23, 2021

Great! And a separate pull request to add those features seems good. Thanks!

@ndevenish ndevenish linked an issue Jan 14, 2022 that may be closed by this pull request
@ndevenish ndevenish merged commit 87c67be into main Jan 14, 2022
@ndevenish ndevenish deleted the correct-array-index branch January 14, 2022 14:47
ndevenish pushed a commit that referenced this pull request Jan 24, 2022
Correct instances where an array-index, which might not start from 0, is
conflated with a scan-point index, which always starts from zero.

Fixes #1929
DiamondLightSource-build-server added a commit that referenced this pull request Jan 24, 2022
Bugfixes
--------

- Correct reporting of phi angle in scan-varying model plots. (#1929)
- ``dials.sequence_to_stills``: correct error in the crystal model for scans that do not start from image 1. (#1933)
- `dials.show`: only show format class if meaningful (i.e. not Format or FormatMultiImage) (#1981)

Improved Documentation
----------------------

- Update some Cosym and resolution-related PHIL descriptions. (#1969)
DiamondLightSource-build-server added a commit that referenced this pull request Jan 25, 2022
Bugfixes
--------

- Correct reporting of phi angle in scan-varying model plots. (#1929)
- ``dials.sequence_to_stills``: correct error in the crystal model for scans that do not start from image 1. (#1933)
- `dials.show`: only show format class if meaningful (i.e. not Format or FormatMultiImage) (#1981)

Improved Documentation
----------------------

- Update some Cosym and resolution-related PHIL descriptions. (#1969)
DiamondLightSource-build-server added a commit that referenced this pull request Jan 25, 2022
Features
--------

- It is now possible to explicitly bootstrap all supported python versions. (`#XXX <https://github.com/dials/dials/issues/XXX>`_)

Bugfixes
--------

- Correct reporting of phi angle in scan-varying model plots. (#1929)
- ``dials.sequence_to_stills``: correct error in the crystal model for scans that do not start from image 1. (#1933)
- `dials.show`: only show format class if meaningful (i.e. not Format or FormatMultiImage) (#1981)

Improved Documentation
----------------------

- Update some Cosym and resolution-related PHIL descriptions. (#1969)
DiamondLightSource-build-server added a commit that referenced this pull request Jan 25, 2022
Features
--------

- It is now possible to explicitly bootstrap all supported python versions. (#1988)

Bugfixes
--------

- Correct reporting of phi angle in scan-varying model plots. (#1929)
- ``dials.sequence_to_stills``: correct error in the crystal model for scans that do not start from image 1. (#1933)
- `dials.show`: only show format class if meaningful (i.e. not Format or FormatMultiImage) (#1981)

Improved Documentation
----------------------

- Update some Cosym and resolution-related PHIL descriptions. (#1969)
DiamondLightSource-build-server added a commit that referenced this pull request Jan 25, 2022
Features
--------

- It is now possible to explicitly bootstrap all supported python versions. (#1988)

Bugfixes
--------

- ``dials.refine``: Fix crash from recording of parameter correlations in ``history.json``. (#1923)
- Correct reporting of phi angle in scan-varying model plots. (#1929)
- ``dials.sequence_to_stills``: correct error in the crystal model for scans that do not start from image 1. (#1933)
- `dials.show`: only show format class if meaningful (i.e. not Format or FormatMultiImage) (#1981)

Improved Documentation
----------------------

- Update some Cosym and resolution-related PHIL descriptions. (#1969)
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.

Scan-points vs array position nastiness
6 participants