Update tf.cross, preserve quaternion dtype, and new quaternion computing function

[ecpoppenheimer]

I made two bug fixes:

1. When the quaternion class needs to create a new quaternion (say in inverse() ) the new quaternion will take the default dtype, instead of the parent's dtype, which will cause a type error if the parent does not use the default dtype. I added code to pass the parent's dtype to the child in these cases.
2. tf.cross seems to not be a valid alias for tf.linalg.cross.

On the version of this library I obtained from pip the keep_dims / keepdims change was not present, but it looks like you did make this change in the repository. Perhaps the version pip knows about isn't updated? (I have no idea how pip works)

I also added a utility function which I find to be the most convenient way of defining a quaternion when rotating a collection of vectors in the same way (i.e. rotating a mesh in CAD without distorting it). You can ignore this if you aren't interested.

[PhilJd]

Thanks a lot for the contribution and sorry for the late reply!
I've added a couple of comments.
Could you also change the title and commit messages to describe which bugs you fix and what new functionality you add instead of the generic 'Minor bug fixes + new code'?
Thanks!

[PhilJd]

Argh github. Seems like it lost most of my comments. Will redo.

[PhilJd]

Thanks a lot for the contribution and sorry for the late reply!
I've added a couple of comments.
Could you also change the title and commit messages to describe which bugs you fix and what new functionality you add instead of the generic 'Minor bug fixes + new code'?
Thanks!

[PhilJd]

Please also add test cases for the new function!

[ecpoppenheimer]

I am still a beginner with git, so probably didn't go about this the correct way. I updated my fork, will those updates transfer to this pull request?

But the really annoying thing is that I seem to have inadvertently added a lot of changes to whitespace in my most recent commit... What do you want to do about that?

[ecpoppenheimer]

I added in the script that I used to test rotation_from_u_to_v, but it isn't configured correctly as a unit test. I am a little unsure how to integrate into your unit test class

[ecpoppenheimer]

I made some really major changes to u_to_v function, to make it compatible with batches and to fix problems, since it wasn't very well tested when I submitted the PR. I strongly suspect there is a better way to handle the inversion special case... We need a quaternion that would take V to -V. I can't shake the suspicion that there is a really simple way to compute a general case -1 quaternion, but I haven't been able to find one and didn't feel up for trying it by hand.

[ecpoppenheimer]

Ah, doesn't work with different float64 objects...

[ecpoppenheimer]

Ok, I changed the Quaternion constructor again so that it now takes a default dtype of None, in which case it tries to use the dtype of wxyz, and if that fails then it falls back to tf.float32.

[ecpoppenheimer]

So I am really not quite sure what to do with the test cases. I never learned how to properly use test cases in my limited programming education. It looks like you are using a built in testing module in TF, is that correct? Does it automatically call class members with test in their name? Would it be acceptable to just package all the code in the test script I wrote into the test class in your test script? I don't want to keep it as it's own file like it is now.

[ecpoppenheimer]

I moved the test cases into your existing test class. Still not sure how that code runs, so I haven't tested the code inside the class, but it does run when it is it's own file.

[PhilJd]

Thanks for making these changes! I've added one more round of comments.
RE testing: you're right that the function is executed if it starts with test (and integrating it into the test class is the right way to go :)). The test should be executed by running python test_tf_quaternion.py, please let me know if that's not the case, then I'll try to look into it.

[PhilJd]

Bt the way, the white spaces are fine, they're enforcing correct pep8 :)

[ecpoppenheimer]

I have implemented the most recent round of changes.

When I tried running the test I got a bunch of errors due to running things in eager mode. Looks like you developed this for the graph execution style of TF. In eager mode tensors no longer have an .eval() method.

It seems like changing the line return self._q.eval(session=session) to
return np.array(self) works (have to then include numpy at the top of the page). But then the testing script throws a ton of errors because shapes are getting messed up all over the place.

So I am really not sure what to do about this. The code works in the limited environment where I need it. Are you able to successfully run the tests?

[ecpoppenheimer]

I have thought a little be more about the special case code in get_rotation_quaternion_from_u_to_v. I need any vector orthogonal to U. U cross x-axis will be either orthogonal or zero if U is parallel to the x-axis. I am detecting this condition as a second special case and then trying U cross Y-axis, which should never fail because U cannot be parallel to both. But maybe it would simplify the code slightly if instead I computed orthogonal = u cross x-axis + u cross y-axis. That should always give a non-zero orthogonal vector, right? It won't be normalized, would have to do that. But that would eliminate the second special case... would only need a single tf.where statement. But now that we are always computing two cross products and need to normalize, it would be slightly less performant.

[PhilJd]

I'm really sorry about the delay, the last two weeks have been extremely busy.
I haven't tried to run these tests in a while and I'm not at my private laptop at the moment but I'll try to take a look later today.

Re tf.where: I think both versions are fine, feel free to go with the one you prefer. The performance should be the same as even for tf.where both branches are evaluated.

[ecpoppenheimer]

I am pretty sure the code itself works just fine. I have been using it for a while now and have not had any issues. The unit testing however is totally borked. I spent a little bit of time today trying to fix them and it is just a mess. The issue is that you wrote this library for graph mode, which has since largely been depreciated. So it seems like a lot will have to change with the unit test class. TF isn't really supporting graph mode compatibility very much. In the last few days I have dealt with a handful of silly issues where core TF functionality just isn't supported moving across the eager / graph mode boundary. Like, you can't actually return a ragged tensor out of a TF function back into eager mode. It just isn't supported, but doesn't crash either. It just gives back a blank tensor. But... once I wrote the three or four lines of code to manually implement a ragged tensor it works just fine.

So... I am recommending you just drop testing in graph mode altogether. If it works in eager mode then the actual quaternion code should just work in graph mode too, I just don't understand the unit tester well enough to get it to test both cases.