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.show for unknown columns of type vec3_double or miller_index #2048

Merged
merged 3 commits into from
Apr 12, 2022

Conversation

phyy-nx
Copy link
Member

@phyy-nx phyy-nx commented Mar 30, 2022

Found by running dials.show on reflection tables modified by cctbx.xfel.merge to include a column named miller_index_asymmetric of type flex.miller_index.

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.

Looks like the right fix for now, but I note dials.show has slightly surprising behaviour for 3-vector columns. For example,

>>> from cctbx.array_family import flex
>>> foo=flex.vec3_double([(1,2,3),(3,2,1),(1,3,2)])
>>> foo.max()
(3.0, 3.0, 3.0)

The max is done on columns separately, so you get a value that is not actually present in the array. This is a bit odd if for example you are looking at Miller indices. I'm not sure how best to approach this, but I appreciate this PR is just fixing a bug, not changing that behaviour.

@phyy-nx
Copy link
Member Author

phyy-nx commented Apr 12, 2022

Agree the max of a miller_index array doesn't seem well defined. But ya, this fixes a specific bug so merging as is. Thanks.

@phyy-nx phyy-nx merged commit f38c12f into main Apr 12, 2022
@phyy-nx phyy-nx deleted the show_fix branch April 12, 2022 21:45
@rjgildea
Copy link
Contributor

Looks like the right fix for now, but I note dials.show has slightly surprising behaviour for 3-vector columns. For example,

>>> from cctbx.array_family import flex
>>> foo=flex.vec3_double([(1,2,3),(3,2,1),(1,3,2)])
>>> foo.max()
(3.0, 3.0, 3.0)

The max is done on columns separately, so you get a value that is not actually present in the array. This is a bit odd if for example you are looking at Miller indices. I'm not sure how best to approach this, but I appreciate this PR is just fixing a bug, not changing that behaviour.

I'm not sure that this behaviour is entirely surprising - it is consistent with numpy array.max(axis=0), which makes sense when you're dealing with primarily columnar data:

import numpy as np
>>> a = np.array([(1,2,3),(3,2,1),(1,3,2),(1,1,1)])
>>> a.max()
3
>>> a.max(axis=0)
array([3, 3, 3])
>>> a.max(axis=1)
array([3, 3, 3, 1])

@phyy-nx
Copy link
Member Author

phyy-nx commented Apr 12, 2022

👍

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