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

transforms new_capi version of gvec_to_xy does not use optional arguments bmat and v_inv #681

Open
donald-e-boyce opened this issue Jul 15, 2024 · 4 comments

Comments

@donald-e-boyce
Copy link
Collaborator

The arguments bmat and v_inv to gvec_to_xy are accepted but not used in the new_capi version of the transforms module. The xf_numpy also has those arguments but handles them.

The bmat matrix converts HKLs to the components of the normal vector in the crystallographic reference frame. If this is not None then the input gvec_c are expected to be HKLs. The v_inv argument accounts for the grain stretch in fitting.

However, I don't see any place where these arguments are actually used in calls to gvec_to_xy.

@donald-e-boyce
Copy link
Collaborator Author

Maybe we should just raise a NotImplementedError if those arguments are used.

@donald-e-boyce
Copy link
Collaborator Author

I don't think you're talking about the matrix bmat, which uses the lattice spacing to convert HKLs to crystal components. In xy_to_gvec, the argument rmat_b refers to the beam reference frame, in which the 3-direction is the beam and eta is the azimuthal angle calculated from (x, y) and the 1-direction vector (eta = 0). It is true the middle column is not used here.

@kevindlewis23
Copy link
Collaborator

You're right, deleted my comment

@psavery
Copy link
Collaborator

psavery commented Jul 22, 2024

@donald-e-boyce Oscar had mentioned that there were some C functions that exist where Joel wanted to add new parameters/features, so he rewrote them in Python and added the new parameter/feature, and this resulted in an inconsistency between the C and Python versions (and the Python version would have to be used when those parameters were required).

Perhaps this is one of those cases?

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

No branches or pull requests

3 participants