8000 Allow real and imag functions to return real TensorValue by ovanvincq · Pull Request #1080 · gridap/Gridap.jl · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Allow real and imag functions to return real TensorValue #1080

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

Merged
merged 5 commits into from
Feb 19, 2025

Conversation

ovanvincq
Copy link
Contributor

This small PR allows the real and imag functions to return real results like the base Julia functions

real(TensorValue(1.0+2im,2.0+3im,3.0+4im,4.0+5im))

returns

  • Without the PR:
TensorValue{2, 2, ComplexF64, 4}(1.0 + 0.0im, 2.0 + 0.0im, 3.0 + 0.0im, 4.0 + 0.0im)
  • With the PR:
TensorValue{2, 2, Float64, 4}(1.0, 2.0, 3.0, 4.0)

Uh oh!

There was an error while loading. Please reload this page.

@Antoinemarteau
Copy link
Contributor

Thanks for the PR @ovanvincq , this is good for me.
Can you just replace the tests for real/imag/conj around here with the following to test the PR ?

v1 = VectorValue(1+1im)
v2 = VectorValue(1)
@test real(v1) == v2 && eltype(real(v1)) == eltype(v2)
@test imag(v1) == v2 && eltype(imag(v1)) == eltype(v2)
v2 = VectorValue(1-1im)
@test conj(v1) == v2 && eltype(conj(v1)) == eltype(v2)

v1 = VectorValue(1+1im, 1+1im)
v2 = VectorValue(1, 1)
@test real(v1) == v2 && eltype(real(v1)) == eltype(v2)
@test imag(v1) == v2 && eltype(imag(v1)) == eltype(v2)
v2 = VectorValue(1-1im, 1-1im)
@test conj(v1) == v2 && eltype(conj(v1)) == eltype(v2)

v1 = VectorValue(1+1im, 1+1im, 1+1im)
v2 = VectorValue(1, 1, 1)
@test real(v1) == v2 && eltype(real(v1)) == eltype(v2)
@test imag(v1) == v2 && eltype(imag(v1)) == eltype(v2)
v2 = VectorValue(1-1im, 1-1im, 1-1im)
@test conj(v1) == v2 && eltype(conj(v1)) == eltype(v2)

v1 = TensorValue(1+1im, 1+1im, 1+1im, 1+1im)
v2 = TensorValue(1, 1, 1, 1)
@test real(v1) == v2 && eltype(real(v1)) == eltype(v2)
@test imag(v1) == v2 && eltype(imag(v1)) == eltype(v2)
v2 = TensorValue(1-1im, 1-1im, 1-1im, 1-1im)
@test conj(v1) == v2 && eltype(conj(v1)) == eltype(v2)

v1 = SymTracelessTensorValue(1+1im, 1+1im)
v2 = SymTracelessTensorValue(1, 1)
@test real(v1) == v2 && eltype(real(v1)) == eltype(v2)
@test imag(v1) == v2 && eltype(imag(v1)) == eltype(v2)
v2 = SymTracelessTensorValue(1-1im, 1-1im)
@test conj(v1) == v2 && eltype(conj(v1)) == eltype(v2)

@JordiManyer
Copy link
Member

Are we sure that the changes to _eltype do not impact performance? I would keep 3 signatures:

  • _eltype(op,r,a)
  • _eltype(op,r,a,b)
  • _eltype(op,r,a...)

@Antoinemarteau
Copy link
Contributor

Oh true

julia> my_eltype(op,r,a...) = typeof(reduce(op, zero.(eltype.(a))))
my_eltype (generic function with 1 method)

julia> my_eltype2(op,r,a,b) = typeof(op(zero(eltype(a)),zero(eltype(b))))
my_eltype2 (generic function with 2 methods)

julia> @code_typed my_eltype(+,(1-im,),(1.0-im),(1-im))
CodeInfo(
1 ─ %1  = Core.tuple(a)::Tuple{Tuple{ComplexF64, Complex{Int64}}}
....
│   %11 = Main.reduce(op, %10)::Any
│   %12 = Main.typeof(%11)::DataType                 # computed at runtime
└──       return %12
) => DataType

julia> @code_typed my_eltype2(+,(1-im,),(1.0-im),(1-im))
CodeInfo(
1 ─     return ComplexF64
) => Type{ComplexF64}

@ovanvincq
Copy link
Contributor Author

Ok, I made the requested corrections.

Copy link
codecov bot commented Feb 17, 2025

Codecov Report

Attention: Patch coverage is 71.42857% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.05%. Comparing base (976c104) to head (b735a63).
Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
src/TensorValues/Operations.jl 71.42% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1080      +/-   ##
==========================================
- Coverage   89.06%   89.05%   -0.02%     
==========================================
  Files         197      197              
  Lines       23927    23938      +11     
==========================================
+ Hits        21310    21317       +7     
- Misses       2617     2621       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JordiManyer
Copy link
Member

@ovanvincq can you add some coverage for the missing lines? It's not critical, but better than not have it. Also, could you add a description of the PR in the NEWS.md file? Thanks!

@ovanvincq
Copy link
Contributor Author

@JordiManyer I added some tests for _eltype and updated NEWS.md

@JordiManyer JordiManyer merged commit 0f2ef80 into gridap:master Feb 19, 2025
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0