Skip to content

Conversation

@adamnemecek
Copy link

@adamnemecek adamnemecek commented May 21, 2019

It's a nice to have.

@ararslan ararslan added needs compat annotation Add !!! compat "Julia x.y" to the docstring needs docs Documentation for this change is required needs news A NEWS entry is required for this change needs tests Unit tests are required for this change labels May 21, 2019
@adamnemecek
Copy link
Author

What should be my next steps?

@ararslan
Copy link
Member

I would:

  • Add tests to ensure correctness. You can use the existing tests for 3-vectors as a guideline.
  • Amend the docstring for cross to mention that it can handle vectors with 7 values.
  • Add a compatibility note in the docstring mentioning that 7-vectors require Julia 1.3. (Check out other docstrings with !!!compat in them as a guide.)
  • Add an entry in the LinearAlgebra section of NEWS.md mentioning this change.

Also: Welcome, and thanks for the contribution!

@ararslan ararslan requested a review from stevengj May 22, 2019 01:59
@stevengj
Copy link
Member

stevengj commented May 22, 2019

This is neat, but would anyone use this in practice? I hesitate to merge code unless there is a realistic application in mind. Who would be happy with code that works in 3 or 7 dimensions but not any other dimensions?

@adamnemecek
Copy link
Author

@ararslan I'll add the things in a couple of days.

@stevengj Cross product is defined only in 3 and 7 dimensions. I know it's weird.
https://en.wikipedia.org/wiki/Seven-dimensional_cross_product

@ViralBShah
Copy link
Member

I wonder if this is better to put into a package. (I wonder which one).

@stevengj
Copy link
Member

stevengj commented May 22, 2019

@adamnemecek, I know that. But just because it can be defined in 7 dimensions doesn't automatically mean that we should include the code — it depends on whether it is useful.

The 3-dimensional cross-product is useful because we live in 3 dimensions and so lots of people do computations with this product in practice. On the other hand, any code that wants to handle 7 dimensions probably either

  1. is using octonions, in which case they should just use an octonion type from https://github.com/JuliaGeometry/Quaternions.jl

  2. needs to handle any number of dimensions, in which case they shouldn't use cross products at all and must frame their algorithms in terms of bivectors or similar.

@adamnemecek
Copy link
Author

@ViralBShah I thought about that but there's no way to overload a function based on array size.

@stevengj You are correct, I'd like to use it for implementation of octonions. However I need a custom octonion implementation for one reason or another. I still feel like it makes sense to add 7 dim cross product, just for the sake of completeness.

@thofma
Copy link
Contributor

thofma commented May 22, 2019

Since it is not unique, why choose this one?

@adamnemecek
Copy link
Author

adamnemecek commented May 22, 2019

Whats not unique.

@thofma
Copy link
Contributor

thofma commented May 22, 2019

There is not "the" seven dimensional cross product.

@StefanKarpinski
Copy link
Member

https://en.wikipedia.org/wiki/Seven-dimensional_cross_product:

while the three-dimensional cross product is unique up to a sign, there are many seven-dimensional cross products

@adamnemecek
Copy link
Author

OK if someone needs a different one they can deal with that. This one is more commonly used than the others.

@StefanKarpinski
Copy link
Member

Do you have a reference for that?

@adamnemecek
Copy link
Author

Is it going to change things If I provide a different one?

@StefanKarpinski
Copy link
Member

It might. If there's one unique preferred cross product in seven dimensions, then that's a decent argument for "well, we might as well have it compute that". For three dimensions, there are two cross products that differ only by sign, and one of those is the standard by convention. I know squat about seven dimensional cross products, but from reading that wikipedia page, the non-uniqueness makes me very reluctant to bake an arbitrary choice in that may or may not be standard. If you can make a convincing case that it is actually standard, that's much more compelling.

@adamnemecek
Copy link
Author

The implementation I went with is the one from wiki. I can take the one from the julia implementation of octonions. I think it makes sense to pick one and be consistent.

@thofma
Copy link
Contributor

thofma commented May 22, 2019

The wikipedia page gives at least two different definitions.

@adamnemecek
Copy link
Author

Do you like any of the other ones better?

@thofma
Copy link
Contributor

thofma commented May 22, 2019

In any case the docstring needs to explain clearly which cross product is given. I don't know what the most compact form would be. Multiplication table?

@adamnemecek
Copy link
Author

OK, I'll add a docstring if I know this will get merged once it's polished.

@StefanKarpinski
Copy link
Member

StefanKarpinski commented May 22, 2019

I think it makes sense to pick one and be consistent.

I'm afraid that's not how it works in the base language. We are not trying to be difficult or argumentative here—by accepting an API in the language, we are committing to it essentially forever. You may well forget about this PR or that you ever worked with seven dimensional cross products in a few years, but I'll be here answering questions about why we chose to implement this particular cross product for decades to come. Before more work is put into this, it should be determined whether there is one standard cross product that it makes sense to bake into the language forever. If not, then this definition belongs in a package which has much more leeway to be opinionated since people can choose which packages they use.

@StefanKarpinski StefanKarpinski added the speculative Whether the change will be implemented is speculative label May 22, 2019
@adamnemecek
Copy link
Author

Imma close it.

@stevengj
Copy link
Member

However I need a custom octonion implementation for one reason or another

If there is something wrong with the type in Quaternions.jl please file an issue there or make a PR.

@adamnemecek
Copy link
Author

adamnemecek commented May 22, 2019

Nothing is wrong with it, I'm trying to implement some clifford algebras and I'd like to able to declare say dual sedenions by composing already existing types.

I was attempting something yesterday that required the cross product to be defined for also 1 and 7 dimensions. Ive abandoned that approach since then.

I checked if julia does the 7 dim cross product and thought it was a simple contribution.

@ViralBShah
Copy link
Member

Thank you for the PR. I definitely learnt something new here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs compat annotation Add !!! compat "Julia x.y" to the docstring needs docs Documentation for this change is required needs news A NEWS entry is required for this change needs tests Unit tests are required for this change speculative Whether the change will be implemented is speculative

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants