Logger.info output during alignment exception

[jaketanderson]

Added a logger.info under get_rotation_matrix for a more informative output than a plain runtime error. The info reads "The structure is already aligned and the denominator is invalid, skipping" when the cross product of the vector and the ref_vector is zero, indicating they are already aligned.

[slochower]

That's a good idea. Is that the only time the RuntimeError will be triggered? You could log that statement if cross-product is zero and then print something else for other triggers of the error? Just a thought.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 4855692e99d7a331a4e1601ddaef836a9abd32b1 into 9bd292d75cda5cecc7dc2ed815d808948fe9c29c - view on LGTM.com

new alerts:

• 1 for Unnecessary pass

[jaketanderson]

That's a good idea. Is that the only time the RuntimeError will be triggered? You could log that statement if cross-product is zero and then print something else for other triggers of the error? Just a thought.

To my knowledge the RuntimeError is triggered only by the division of zero, which occurs only when np.linalg.norm(np.cross(vector, ref_vector)) is zero. I suppose it could cause problems in the future if get_rotation_matrix has some other functionalities. In that case, any RuntimeError might log the message, implying the issue is that the structures are already aligned. If the error were caused by some bad code in that function, though, it might be harder to debug.

I'll go ahead and change it so that it logs when np.linalg.norm(np.cross(vector, ref_vector)) is zero, for the sake of generality.

[jaketanderson]

In the new commit, if the structures are already aligned and the try-except doesn't run will this create issues with x? I assumed it won't, but I don't know for sure.

[slochower]

In this version, it looks like x will be never be defined if you go into the if statement, so maybe you just want to return at this point? I'm slightly hazy about the code paths now. You can try adding test cases to verify! I think there should already be a couple in there for alignment.

[jaketanderson]

I removed the conditionality of the definition of x so the runtime error will still appear when the axes are already aligned (not my goal but it is still an improvement), but it will be accompanied by a log saying the exception is harmless and due to the structure being pre-aligned.

[slochower]

It's up to you if you're happy with the way it is currently. I'm open to other changes but also happy if you think this is good.

[jaketanderson]

I think I like where it is now. I'm a novice to programming so I'm happy I was able to make an improvement, even if it's just adding a more descriptive log :)

[slochower]

Any comments @jeff231li ?

[jeff231li]

@jaketanderson thank you for bringing this up. I think we can simplify the code for get_rotation_matrix further. We can check if the cross product is zero at the start of the function and return an identity matrix. This way we can get rid of the try-except block. What do you think?

def get_rotation_matrix(vector, ref_vector):
    if np.linalg.norm(np.cross(vector, ref_vector)) == 0:
        logger.info("The structure is already aligned and the denominator is invalid, returning identity matrix.")
        return np.identity(3)

    # Find the axis between the mask vector and the axis using cross and dot products.
    x = ...
    theta = ...
    A = ...
    rotation_matrix = ...

    return rotation_matrix

[jaketanderson]

@jeff231li Great, I'm glad the try-except can be avoided. Returning an identity matrix of 3x3 will result in a matrix that applies no rotation to the structure it is called on, correct? I don't see any issues with that. Seems like a solid improvement.

[jeff231li]

Awesome! lgtm.

[slochower]

🎉

[slochower]

@jaketanderson feel free to open a PR to edit the README and add your name to the authors list.