-
Notifications
You must be signed in to change notification settings - Fork 77
Enable dims argument for weighted norms #590
base: master
Are you sure you want to change the base?
Conversation
bors r+ |
590: Enable dims argument for weighted norms r=simonbyrne a=simonbyrne Previously this threw an error Co-authored-by: Simon Byrne <[email protected]>
Build failed |
Codecov Report
@@ Coverage Diff @@
## master #590 +/- ##
=========================================
+ Coverage 68.02% 68.2% +0.18%
=========================================
Files 82 82
Lines 5620 5618 -2
=========================================
+ Hits 3823 3832 +9
+ Misses 1797 1786 -11
Continue to review full report at Codecov.
|
1a8bd84
to
6f18f44
Compare
bors try |
tryBuild failed |
Previously this threw an error
6f18f44
to
ed464b6
Compare
bors r+ |
590: Enable dims argument for weighted norms r=simonbyrne a=simonbyrne Previously this threw an error Co-authored-by: Simon Byrne <[email protected]>
Build failed |
@@ -488,7 +488,7 @@ function weighted_norm_impl(Q::SubArray{FT, N, A}, W, ::Val{p}, dims::Colon) whe | |||
if isfinite(p) | |||
accum += W[i, 1, k] * abs(Q[i, j, k]) ^ p | |||
else | |||
waQ_ijk = W[i, j, k] * abs(Q[i, j, k]) | |||
waQ_ijk = W[i, 1, k] * abs(Q[i, j, k]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this being changed? (And why is the line above read with a 1
also: accum += W[i, 1, k] * abs(Q[i, j, k]) ^ p
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weights are set up to have size(weights, 2) == 1
@simonbyrne, is this PR still relevant? |
yes, we still need to fix it. |
@test norm(QAW, Inf) ≈ norm(globalA, Inf) | ||
|
||
if ArrayType == Array | ||
# TODO: make this work with CuArrays |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NLsolve.jl needs the same thing
Previously this threw an error