Skip to content

Conversation

@dpo
Copy link
Member

@dpo dpo commented Nov 13, 2019

closes #47

@dpo dpo requested a review from abelsiqueira November 13, 2019 05:39
@abelsiqueira abelsiqueira reopened this Nov 13, 2019
Copy link
Member

@abelsiqueira abelsiqueira left a comment

Choose a reason for hiding this comment

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

Thanks, Dominique. I've asked for an additional test, because of my coverage fever, but I'm gonna approve the PR.

end
@test nprod(op) == nprods
@test ntprod(op) == ntprods
@test nctprod(op) == nctprods
Copy link
Member

Choose a reason for hiding this comment

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

Could add a test here for transpose, adjoint and conjugate operators? So we can cover these lines. 💯

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. Done. Thanks.

@codecov
Copy link

codecov bot commented Nov 13, 2019

Codecov Report

Merging #119 into master will increase coverage by 0.08%.
The diff coverage is 97.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
+ Coverage   96.58%   96.66%   +0.08%     
==========================================
  Files           6        6              
  Lines         527      570      +43     
==========================================
+ Hits          509      551      +42     
- Misses         18       19       +1
Impacted Files Coverage Δ
src/lsr1.jl 98.48% <100%> (+0.09%) ⬆️
src/LinearOperators.jl 94.38% <100%> (+0.31%) ⬆️
src/PreallocatedLinearOperators.jl 100% <100%> (ø) ⬆️
src/lbfgs.jl 97.97% <100%> (+0.08%) ⬆️
src/adjtrans.jl 98.66% <94.73%> (-1.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c5c904...707f94f. Read the comment docs.

@dpo
Copy link
Member Author

dpo commented Nov 13, 2019

I made a few changes and introduced an "API" to increase counters. That'll be useful in the next PR.

@dpo dpo merged commit f662283 into master Nov 13, 2019

increase_nprod(op::AbstractLinearOperator) = (op.nprod += 1)
increase_ntprod(op::AbstractLinearOperator) = (op.ntprod += 1)
increase_nctprod(op::AbstractLinearOperator) = (op.nctprod += 1)
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to define it for Adjoint, Transpose and Conjugate as well

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. It's a non-issue at this point because we're not wrapping operators into other operators, but it will be done in the next PR.

@abelsiqueira abelsiqueira deleted the counters branch November 13, 2019 17:27
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.

Add counters for number of products performed

3 participants