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

Add 5 arguments mul! #146

Closed
wants to merge 1 commit into from

Conversation

amontoison
Copy link
Member

No description provided.

@coveralls
Copy link

coveralls commented Mar 31, 2020

Coverage Status

Coverage increased (+0.02%) to 96.349% when pulling 8459f58 on amontoison:mul! into 174c648 on JuliaSmoothOptimizers:master.

@codecov
Copy link

codecov bot commented Mar 31, 2020

Codecov Report

Merging #146 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #146      +/-   ##
==========================================
+ Coverage   96.32%   96.34%   +0.02%     
==========================================
  Files          14       14              
  Lines         625      630       +5     
==========================================
+ Hits          602      607       +5     
  Misses         23       23              
Impacted Files Coverage Δ
src/operations.jl 100.00% <100.00%> (ø)
src/lsr1.jl 98.55% <0.00%> (+0.04%) ⬆️

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 560071b...8459f58. Read the comment docs.

@abelsiqueira
Copy link
Member

abelsiqueira commented Apr 1, 2020

Is this a 1.3 thing? If so, we should also update the version because we drop support for Julia. I'm just not sure if it should be major or minor.

@amontoison
Copy link
Member Author

Yes, it's a feature of Julia 1.3. I think release 1.1 for LinearOperators is appropriate, no?

appveyor.yml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
# y ⇽ α * op * x + β * y
function mul!(y :: AbstractVector, op :: AbstractLinearOperator, x :: AbstractVector, α :: Number, β :: Number)
β ≠ 0 && (y .*= β)
α ≠ 0 && (y .+= α .* (op * x))
Copy link
Member

Choose a reason for hiding this comment

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

Does this assume that op Is able to directly update y? That's how it works with matrices I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, one vector is allocated (or already preallocated) for op * x product and after y is updated.

@dpo
Copy link
Member

dpo commented Apr 1, 2020

Maybe operators should be defined by the 5-arg mul!. What do you think?

@amontoison
Copy link
Member Author

Yes, it's a good idea.

@abelsiqueira abelsiqueira requested a review from dpo April 9, 2020 14:22
@dpo
Copy link
Member

dpo commented Apr 9, 2020

Frankly, I have to say that I strongly dislike the mul!() notation. The first and foremost reason why this package was created is to avoid such notation and be able to simply write A * x. If this notation propagates into Krylov.jl, it defeats the entire purpose.

@abelsiqueira
Copy link
Member

I think it's just for compatibility in other packages, in case someone decides to use it. Since matrices will support them, we might as well have it.
We should prevent using it in our packages, especially because It doesn't seem to improve speed in our case.
By the way, I just realized that we don't need to drop < 1.3, since defining 5-arg mul! shouldn't break anything, right @amontoison?

@dpo
Copy link
Member

dpo commented Apr 9, 2020

Compatibility with what though? Has there been demand for this? If it still allocates a vector, what is the advantage?

@amontoison
Copy link
Member Author

Yes, you're right Abel, we don't need to drop < 1.3. I think it's still relevant to have it like the 3-arguments mul!. I have something in mind for Krylov.jl that use it without it's explicit form with a macro.

@dpo
Copy link
Member

dpo commented Apr 9, 2020

I continue to think that it would be better to define operators with a 5-arg prod!() instead of the prod() that we are using now, so that we can simply write A * x. No macro needed.

Copy link
Member

@dpo dpo left a comment

Choose a reason for hiding this comment

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

Let's put this on hold temporarily.

@amontoison
Copy link
Member Author

superseded by #180

@amontoison amontoison closed this Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants