RFC: Fixed scale and scale1 methods to not assume commutativity. Issue #13690

[sarvghotra]

i.e. distinguish between pre- or post-multiplications.

[StefanKarpinski]

+1 cc: @andreasnoack, @jiahao

[jiahao]

Looks good, thank you!

[jiahao]

There is a latent bug reporting issue in these scale methods too - the scale!(C, a, B) methods would fail with an InexactError in setindex! if the numeric type of a*B[1] is not assignable to C[1]. An example is C::Vector{Int}, B::Vector{Float64}, and a = 2.0+0.1im. Should we also modify these methods to catch this kind of error earlier?

[andreasnoack]

When working with the ! versions, it is the responsibility of the programmer to provide a suitable container and I think we return InexactErrors consistenty enough that it is informative for the source of the error.

[jiahao]

In which case, this PR is fine. Would be good to include some tests with simple noncommutative algebras.

[sarvghotra]

@jiahao To test what in this case? Scales already have tests.

[tkelman]

They already have tests, but they weren't covering any non-commutative cases that this PR fixes. Try to find some examples with simple types that would fail on master but pass with this PR.

[sarvghotra]

@tkelman AppVeyor build failed and showed the following. Its just due to not installing Quaternions pakage, though I have included this in the test files.

Exception running test linalg/generic : On worker 3: LoadError: ArgumentError: Quaternions not found in path. Run Pkg.add("Quaternions") to install the Quaternions package in require at loading.jl:321 [inlined code] from essentials.jl:114 in include_string at loading.jl:355 in include_from_node1 at loading.jl:404 [inlined code] from util.jl:179 in runtests at C:\projects\julia\test\testdefs.jl:7 in anonymous at C:\projects\julia\test\runtests.jl:36 in anonymous at multi.jl:1005 in run_work_thunk at multi.jl:709 [inlined code] from multi.jl:1005 in anonymous at task.jl:63

What to do about this ?

[tkelman]

You added using Quaternions but that doesn't install the package. How long would a bare-bones testing-only type definition be to get the minimum amount of functionality necessary to demonstrate the problem?

[sarvghotra]

@tkelman Could you please rephrase your question?

[tkelman]

Can you just include the minimum quaternion type definition and non-commutative multiplication inline in the tests for this?

[tkelman]

The quaternion type for testing belongs under test/, not under base/

[tkelman]

thanks for the reminder