Consistency of call signatures in gemmi.Op and vectorized apply_to_hkl() and phase_shift() about gemmi HOT 6 CLOSED

Hi Jack,

regarding phase_shift() -- thanks for pointing it out, I've changed it as you proposed.

regarding vectorization in Python - I was considering vectorization of some functions, because after reading pybind11 it seems like an easy thing to do:
https://pybind11.readthedocs.io/en/stable/advanced/pycpp/numpy.html#vectorizing-functions
But I haven't experimented with it yet.
I don't know if working with reflection data in numpy arrays will be a common use case :-)
You are probably the first person doing this, these functions are not even documented yet.
But on the other hand, these functions are good candidates for vectorization.
In your use case, are these the only functions that would benefit from vectorization?

from gemmi.

Hi Marcin,

Thanks for taking care of this so quickly.

Working with reflection data in numpy arrays may not be a common use case yet, but I hope it will be some day! In general, I think any function that maps HKL indices in a pairwise manner to something (either other HKLs or floats) ought to be a good candidate for vectorization. I've been using 3 functions that meet that criteria:

• Op.apply_to_hkl()
• Op.phase_shift()
• UnitCell.calculate_d()

Looking at the call signature for UnitCell.calculate_d(), it takes 3 sequential float arguments:

calculate_d(self: gemmi.UnitCell, hk: float, k: float, l: float) -> float

Please let me know if you think it also makes sense to change the signature of this one as well:

calculate_d(self: gemmi.UnitCell, hkl: List[float[3]]) -> float

If it ends up being straightforward to vectorize these functions (the pybind11 documentation certainly makes it seem like close to a one-liner), I think these changes in call signatures will make the syntax for these functions most consistent.

from gemmi.

Hi Jack,
I changed calculate_d and calculate_1_d2, but vectorization is not that simple.
It would be simple if the arguments were single numbers (i.e. h, k and l separately, not [h,k,l]), and if the result would also be a single number. So apply_to_hkl() is particularly problematic because of the return value.
Maybe if hkl was passed around in some structured numpy datatype it would work, but I'm not sure.

from gemmi.

Got it -- thanks for the changes in call signatures.

For now, I have vectorized versions of in my code that use attributes of gemmi.Op and numpy functions. This is definitely an example of "passing around as structured numpy datatypes", though, as you mentioned.

for example:

def apply_to_hkl(H, op):
    """
    H : np.ndarray
        (n x 3) numpy array of Miller indices
    op : gemmi.Op
        symmetry operation
    """
    return np.floor_divide(np.matmul(H, op.rot), op.DEN)

from gemmi.

Neat! That's probably the best solution.
Let me know when your package is publicly available.

from gemmi.

I'm going to close this issue -- it may be worth revisiting vectorization in the future, but I will keep it in our package for now.

I will be sure to let you know when we release our code!

from gemmi.