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.

Sign up for GitHub

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

Jump to bottom

Beam.get_direction: proposed name change #6

Closed
dagewa opened this issue Feb 6, 2018 · 3 comments · Fixed by #68
Closed

Beam.get_direction: proposed name change #6

dagewa opened this issue Feb 6, 2018 · 3 comments · Fixed by #68
Assignees
@dagewa

Comments

@dagewa
Copy link
Member

dagewa commented Feb 6, 2018

The dxtbx Beam class has a method called Beam.get_direction, but unfortunately this invites confusion. Without looking into the details of Beam, a developer might reasonably expect this to return a unit vector pointing in the direction of propagation of the beam. In fact, it returns the inverse of that, i.e. the sample-to-source direction (the method Beam.get_unit_s0 does return what is expected). This trap has claimed at least one victim (cctbx/cctbx_project#133) and needs some care to disentangle.

The imgCIF coordinate system describes this direction using the 'source axis'.
http://www.iucr.org/__data/iucr/cifdic_html/2/cif_img.dic/Caxis.html

Axis 3 (Z): The Z-axis is derived from the source axis which goes from
the sample to the source. The Z-axis is the component of the source axis
in the direction of the source orthogonal to the X-axis in the plane
defined by the X-axis and the source axis.

Perhaps we could come up with a better method name that doesn't conceal an ambiguity. Maybe Beam.get_unit_source_axis or Beam.get_source_direction?
@nksauter
Copy link

nksauter commented Feb 7, 2018 via email •
edited by ndevenish
Loading

@graeme-winter
Copy link
Collaborator

Anyone interested in implementing this change? Will require changes to a bunch of dependent code in dials, xia2, cctbx.xfel etc, but not insurmountable.

@Anthchirp Anthchirp transferred this issue from cctbx/cctbx_project Apr 18, 2019
@ndevenish ndevenish changed the title dxtbx Beam.get_direction: proposed name change Beam.get_direction: proposed name change Apr 18, 2019
@dagewa dagewa self-assigned this Jul 17, 2019
@dagewa
Copy link
Member Author

dagewa commented Jul 17, 2019

As originator, I'll take this on.

dagewa added a commit that referenced this issue Jul 17, 2019
@dagewa
Rename Beam.get_direction
90e3131 
Beam.get_direction --> Beam.get_sample_to_source_direction.
Fixes #6
dagewa added a commit to cctbx/cctbx_project that referenced this issue Jul 17, 2019
@dagewa
Sync with changes for cctbx/dxtbx#6
9602d0d
dagewa added a commit to dials/dials that referenced this issue Jul 17, 2019
@dagewa
Sync with changes for cctbx/dxtbx#6
807d8eb
dagewa added a commit to xia2/xia2 that referenced this issue Jul 17, 2019
@dagewa
Sync with changes for cctbx/dxtbx#6
e1fc5b4 
This seems to be the only use of Beam.get_direction in xia2
(in dead code?)
@dagewa dagewa mentioned this issue Jul 17, 2019
Merged
dagewa added a commit that referenced this issue Sep 2, 2019
@dagewa
Rename Beam.get_direction
5c7e92a 
Beam.get_direction --> Beam.get_sample_to_source_direction.
Fixes #6
@dagewa dagewa closed this as completed in #68 Sep 2, 2019
dagewa added a commit that referenced this issue Sep 2, 2019
@dagewa @Anthchirp
Rename Beam.get_direction (#68)
0348ec5 
* Rename Beam.get_direction

Beam.get_direction --> Beam.get_sample_to_source_direction.
Fixes #6

* Inject deprecated get_direction method with a warning

* Second attempt at injector

* Update model/__init__.py

Co-Authored-By: Markus Gerstel <2102431+Anthchirp@users.noreply.github.com>
dagewa added a commit to cctbx/cctbx_project that referenced this issue Sep 2, 2019
@dagewa
Sync with changes for cctbx/dxtbx#6
da3ae0f
dagewa added a commit to xia2/xia2 that referenced this issue Sep 2, 2019
@dagewa
Sync with changes for cctbx/dxtbx#6
f235b19 
This seems to be the only use of Beam.get_direction in xia2
(in dead code?)
dagewa added a commit to dials/dials that referenced this issue Sep 2, 2019
@dagewa
Sync with changes for cctbx/dxtbx#6
ffe37ce
rjgildea pushed a commit to dials/dials that referenced this issue Sep 10, 2019
@dagewa @rjgildea
Sync with changes for cctbx/dxtbx#6
c1bc3a0
Anthchirp added a commit that referenced this issue Nov 13, 2019
@Anthchirp
Remove deprecated classes/functions/parameters
f923c81 
* out= parameter removed for xparm_xds()
* verbose= parameter removed for classes
  * FormatChecker
  * DataBlockTemplateImporter
  * DataBlockFilenameImporter
* Beam.get_direction() removed (#6, #68)
* ExperimentListDumper removed (#54)
* Registry.lookup() removed (#34)
* Registry.Registry removed (#34)
* Add formal deprecation warning for the
  as_str= parameter in XDS_INP()
  (cf. b2533d5)
@Anthchirp Anthchirp mentioned this issue Nov 13, 2019
Merged
Anthchirp added a commit that referenced this issue Nov 19, 2019
@Anthchirp
Remove deprecated classes/functions/parameters (#115)
bc0b3d3 
* out= parameter removed for xparm_xds()
* verbose= parameter removed for classes
  * FormatChecker
  * DataBlockTemplateImporter
  * DataBlockFilenameImporter
* Beam.get_direction() removed (#6, #68)
* ExperimentListDumper removed (#54)
* Registry.lookup() removed (#34)
* Registry.Registry removed (#34)
* Add formal deprecation warning for the
  as_str= parameter in XDS_INP()
  (cf. b2533d5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dagewa dagewa

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Rename Beam.get_direction cctbx/dxtbx
3 participants
@graeme-winter @dagewa @nksauter