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

New calculus rules #10

Merged
merged 12 commits into from
Mar 21, 2019
Merged

New calculus rules #10

merged 12 commits into from
Mar 21, 2019

Conversation

nantonel
Copy link
Member

@nantonel nantonel commented Mar 14, 2019

  • Ax_mul_Bx --> Generalizes NonLinearCompose
  • Axt_mul_Bx
  • Ax_mul_Bxt
  • HadamardProd --> Generalizes Hadamard

Hadamard & NonLinearCompose will be deprecated in future version of AbstractOperators.

@codecov-io
Copy link

codecov-io commented Mar 14, 2019

Codecov Report

Merging #10 into master will decrease coverage by 0.46%.
The diff coverage is 97.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #10      +/-   ##
==========================================
- Coverage   84.27%   83.81%   -0.47%     
==========================================
  Files          45       49       +4     
  Lines        1475     1643     +168     
==========================================
+ Hits         1243     1377     +134     
- Misses        232      266      +34
Impacted Files Coverage Δ
src/AbstractOperators.jl 100% <ø> (ø) ⬆️
src/calculus/HadamardProd.jl 100% <100%> (ø)
src/linearoperators/LBFGS.jl 96% <100%> (ø) ⬆️
src/calculus/Hadamard.jl 93.33% <100%> (+0.22%) ⬆️
src/calculus/Ax_mul_Bx.jl 100% <100%> (ø)
src/calculus/NonLinearCompose.jl 100% <100%> (ø) ⬆️
src/calculus/Ax_mul_Bxt.jl 94.11% <94.11%> (ø)
src/calculus/Axt_mul_Bx.jl 95.45% <95.45%> (ø)
src/calculus/Sum.jl 87.23% <95.83%> (-1.9%) ⬇️
src/nonlinearoperators/Pow.jl 60% <0%> (-25.72%) ⬇️
... and 39 more

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 8e0fbd8...89ffe87. Read the comment docs.


Compose opeators such that:

`(Ax).*(Bx)`
Copy link
Member

@lostella lostella Mar 20, 2019

Choose a reason for hiding this comment

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

This passage is not really clear. Maybe mention explicitly state that P*x = A*x .* B*x where P is the operator being constructed.

Also, this operator seems to in fact reduce to the usual Hadamard product between matrices in the linear case: is the nonlinear case a well known generalization? Just asking out of curiosity, to make sure there’s no confusion with the naming

Copy link
Member Author

@nantonel nantonel Mar 20, 2019

Choose a reason for hiding this comment

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

I'll make the docs more clear, thanks for pointing that out!

Not sure how much the nonlinear generalization is known, however it is, as usual, the chain rule applied 😄

Anyway the name could be perhaps improved with something like HadamardCompose, but it is very lengthy.

By the way my idea is to deprecate the other calc rule Hadamard since HadamardProd generalises it. So definitively it won't be confused with that one.
(updated first comment ⬆️ )

Copy link
Member

@lostella lostella left a comment

Choose a reason for hiding this comment

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

Food for thought: what is missing to be able to simply express these rules using composition? It seems to me like these are highly redundant.

@nantonel
Copy link
Member Author

Yes I agree there’s a high level of redundancy... probably all the Ax_mul_Bx variants can be merged into a single one! But I still don’t have a clear idea on how! Maybe a new calculus rule that transposes the output would do the job... future work though!

@nantonel nantonel merged commit e16bbe4 into kul-optec:master Mar 21, 2019
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