Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#34234 normalize(a) for multidimensional arrays #34239

Merged
merged 10 commits into from
Jan 7, 2020
Merged

#34234 normalize(a) for multidimensional arrays #34239

merged 10 commits into from
Jan 7, 2020

Conversation

ssikdar1
Copy link
Contributor

@ssikdar1 ssikdar1 commented Jan 2, 2020

PR to try and resolve issue #34234

  • Change the type signatures and documentation from AbstractVector to AbstractArray
  • Change this line from v[1] to first(v). (This is a bug anyway, since v[1] is wrong for non 1-based arrays.)
  • Change the documentation to refer to "array" rather than "vector" and use a rather than v. (changed docstrings)
  • Add a few tests. (not sure if i need more?)
  • Add a NEWS item.

Testing:
added unit test to stdlib/LinearAlgebra/test/generic.jl
ran ./julia stdlib/LinearAlgebra/test/generic.jl with no errors.

...
Test Summary:                         | Pass  Total
normalize for multidimensional arrays |    2      2
...

Testing in the shell:

julia> using LinearAlgebra;

julia> arr = [ [1.0 0.0]; [0.0 1.0] ]
2×2 Array{Float64,2}:
 1.0  0.0
 0.0  1.0

julia> @show arr
arr = [1.0 0.0; 0.0 1.0]
2×2 Array{Float64,2}:
 1.0  0.0
 0.0  1.0

julia> @show normalize(arr)
normalize(arr) = [0.7071067811865475 0.0; 0.0 0.7071067811865475]
2×2 Array{Float64,2}:
 0.707107  0.0
 0.0       0.707107

julia> @show normalize!(copy(arr))
normalize!(copy(arr)) = [0.7071067811865475 0.0; 0.0 0.7071067811865475]
2×2 Array{Float64,2}:
 0.707107  0.0
 0.0       0.707107

julia> @show normalize(arr) == normalize!(copy(arr))
normalize(arr) == normalize!(copy(arr)) = true
true

julia> arr = [[1.0 0.0 0.0]; [0.0 1.0 0.0]]
2×3 Array{Float64,2}:
 1.0  0.0  0.0
 0.0  1.0  0.0

julia> @show normalize(arr) == normalize!(copy(arr))
normalize(arr) == normalize!(copy(arr)) = true
true

julia> a = [1,0,0]
3-element Array{Int64,1}:
 1
 0
 0

julia> @show normalize(a) == normalize!(copy(a))
normalize(a) == normalize!(copy(a)) = true
true

@oscardssmith
Copy link
Member

Should this be backported? It seems simple and is partially a bug fix

@StefanKarpinski
Copy link
Sponsor Member

StefanKarpinski commented Jan 3, 2020

No, this is definitely a feature. The bugfix part could be backported.

Case for OffsetArray where A[1] would fail but
first(A) would not. Also some more test cases to
compare with the vector case
@ssikdar1
Copy link
Contributor Author

ssikdar1 commented Jan 3, 2020

The CI failures seem to be complaining about the Makefile and seem to be unrelated?

make: *** No rule to make target 'win-extras'.  Stop.

@stevengj
Copy link
Member

stevengj commented Jan 4, 2020

LGTM. Thanks for tackling this, @ssikdar1!

NEWS.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jw3126 jw3126 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks very much @ssikdar1 for doing this!

@andreasnoack andreasnoack added the domain:linear algebra Linear algebra label Jan 7, 2020
@andreasnoack andreasnoack merged commit 8f9dd5d into JuliaLang:master Jan 7, 2020
KristofferC pushed a commit that referenced this pull request Apr 11, 2020
* Add support normalize multi dim arrays

* remove trailing whitespace from test

* var name v => a for inner function

* Update normalize tests

Case for OffsetArray where A[1] would fail but
first(A) would not. Also some more test cases to
compare with the vector case

* add NEWS item

* make docstring example w/ array  more julia-thonic

* reduce redundant test cases

* add test for normalize on Int64 array

* add 0 1 and high dim test cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants