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

miscellaneous updates #66

Merged
merged 16 commits into from
Sep 28, 2019
Merged

miscellaneous updates #66

merged 16 commits into from
Sep 28, 2019

Conversation

dkarrasch
Copy link
Member

@dkarrasch dkarrasch commented Sep 2, 2019

  • add size(A, i)
    I realized we more often use size(A, i) instead of size(A), and the interface requests to have size(A), and size(A, i) computes by default size(A)[i]. As for the API, I think this is fine, but I felt we should have specialized methods for individual sizes whenever possible to save unnecessary computations. In all cases that effort is trivial compared to others, but still in some cases there is a little work to be done.
  • make FunctionMap adjoint/transpose-invariant
    We were handling adjoints/transposes of FunctionMaps a bit loose. We never checked at construction whether the adjoint is defined, and we never checked whether the adjoint is also in-place. Since all information is available, I thought we may as well construct a new FunctionMap, and at that point guess again whether fc is also in-place or not. Also, this saves us one hierarchical level. EDIT: That doesn't work well. Instead, I added some test. Strictly speaking, we still can define a FunctionMap where f is in-place, and fc isn't. This is still not caught a priori, but will yield an error at multiplication.
  • require one-based indexing in multiplications that index into the vectors
    I think we were a bit too loose at that point, compared to our signature. Most linalg operations in Base are restricted to such vectors anyway.
  • make alpha and beta true and and false per default, consistent with LinearAlgebra's 5-arg mul!.
  • add docstrings

@coveralls
Copy link

coveralls commented Sep 2, 2019

Coverage Status

Coverage decreased (-0.2%) to 94.484% when pulling 67c762d on miscupdates into 98cc86a on master.

@codecov
Copy link

codecov bot commented Sep 2, 2019

Codecov Report

Merging #66 into master will decrease coverage by 0.41%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #66      +/-   ##
=========================================
- Coverage   98.62%   98.2%   -0.42%     
=========================================
  Files           8       8              
  Lines         435     445      +10     
=========================================
+ Hits          429     437       +8     
- Misses          6       8       +2
Impacted Files Coverage Δ
src/LinearMaps.jl 98.61% <0%> (-1.39%) ⬇️
src/blockmap.jl 98.59% <100%> (+0.04%) ⬆️
src/wrappedmap.jl 100% <100%> (ø) ⬆️
src/functionmap.jl 94.44% <100%> (-2.7%) ⬇️
src/composition.jl 98.38% <100%> (ø) ⬆️
src/linearcombination.jl 100% <100%> (ø) ⬆️
src/transpose.jl 100% <100%> (ø) ⬆️
src/uniformscalingmap.jl 96.36% <100%> (+0.06%) ⬆️

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 c435822...22a9666. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 2, 2019

Codecov Report

Merging #66 into master will decrease coverage by 0.17%.
The diff coverage is 96.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
- Coverage   97.67%   97.49%   -0.18%     
==========================================
  Files           8        8              
  Lines         473      479       +6     
==========================================
+ Hits          462      467       +5     
- Misses         11       12       +1
Impacted Files Coverage Δ
src/blockmap.jl 98.58% <100%> (+0.03%) ⬆️
src/functionmap.jl 97.22% <100%> (+0.07%) ⬆️
src/composition.jl 98.38% <100%> (ø) ⬆️
src/linearcombination.jl 96.92% <100%> (ø) ⬆️
src/transpose.jl 88.88% <100%> (ø) ⬆️
src/uniformscalingmap.jl 96.36% <100%> (+0.06%) ⬆️
src/LinearMaps.jl 98.61% <88.88%> (-1.39%) ⬇️

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 98cc86a...67c762d. Read the comment docs.

@Jutho
Copy link
Collaborator

Jutho commented Sep 6, 2019

Lot's of activity here. I have been of the radar the last two weeks. I will try to start reviewing all of this next week; but I have other awaiting duties as well.

Regarding the size specialization; I think this code addition is not necessary. Surely the compiler will inline size(linearmap, 1) and eliminate the dead code associated with the creation of the tuple and computing its second element, only to retain the first element (and likewise when only the second element is required).

@Jutho
Copy link
Collaborator

Jutho commented Sep 6, 2019

Regarding the transpose/adjoint of a functionmap, I don't know what the check really buys. Suppose you did not define fc,maybe you still want to construct B=A', only then to later only act with B'.

We could try to check that ismutating is compatible with both f and fc in the constructor of FunctionMap though.

@dkarrasch
Copy link
Member Author

Great to have you back and your eyes on it! The idea of this PR is to experiment with some potential changes and see whether they work at all, and finally cherry-pick whatever we feel is worth it. I don't have enough (maybe any) knowledge about what the compiler can eliminate and what not, but since this is not a great deal anyway, I'll remove the size stuff from this PR.

Making adjoint/transpose return a new FunctionMap is what I have experimented with in 44e7098, until I realized that in some cases we don't require fc to be defined and still compute the action of the adjoint/transpose. The good thing is that there haven't been any complaints about something not working, so leaving some things (for now) as they are, is not a bad solution.

@dkarrasch
Copy link
Member Author

Perhaps you're right. The FunctionMap-related transposition/adjoint was working well, and the additional tests at construction don't buy anything. Eventually, the same error is thrown, and potentially users don't even run into the error when they transpose twice. I'll remove that part, and make a mental note that we may think about checking the in-place-ness of both functions, but keep that for another PR. That's not urgent.

@dkarrasch
Copy link
Member Author

Ready for review. 😉

@Jutho
Copy link
Collaborator

Jutho commented Sep 27, 2019

Looks great; no comments; ready to merge.

@dkarrasch dkarrasch merged commit 95cc89c into master Sep 28, 2019
@dkarrasch dkarrasch deleted the miscupdates branch September 28, 2019 11:54
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