WIP: review before version 3 #119
Conversation
Codecov Report
@@ Coverage Diff @@
## master #119 +/- ##
==========================================
- Coverage 99.79% 99.78% -0.01%
==========================================
Files 13 13
Lines 973 950 -23
==========================================
- Hits 971 948 -23
Misses 2 2
Continue to review full report at Codecov.
|
src/functionmap.jl
Outdated
|
|
||
| # properties | ||
| Base.size(A::FunctionMap) = (A.M, A.N) | ||
| Base.parent(A::FunctionMap) = (A.f, A.fc) | ||
| Base.parent(A::FunctionMap) = (A.f, A.fc) # is this meanigful/useful? |
There was a problem hiding this comment.
IIRC the reason for parent was for manual inspection of wrapped maps, because show is now rather sparse. It may be used in the package only rarely, though.
There was a problem hiding this comment.
perhaps any parent-like method would be most useful if it returned all the arguments needed to recreate the object. at a minimum that would include size too. maybe a named tuple would be more useful too.
not sure i would use it much though.
There was a problem hiding this comment.
From Base:
help?> Base.parent
parent(A)
Return the underlying "parent array”. ...
So I see this making sense for WrappedMap, but other than that I am not entirely sure. Where is this used?
There was a problem hiding this comment.
I think we use parent only somewhere in kronecker.jl, and then potentially in an inconsistent way. It's not obvious what parent should return given the wide range of LinearMap subtypes. I'd be fine with actually removing it. I introduced this when I cut down the show method to show much less details, and then I used it in one or two places as it was handy. But this is not really crucial.
There was a problem hiding this comment.
Another decision to be made: what shall we do with parent? Just remove? I think it's used internally only in one spot in the kronecker code, and could be worked around with a ternary if-clause, as I had it originally.
|
blockmap was a though one, as i was not familiar with the code. Kronecker will have to wait for another day (hopefully tomorrow). |
|
I've left some more questions. I am otherwise finished; this package has evolved very nicely and has become very mature. I like the show implementation, might mimic this design in other packages where a similar structure could be useful. I did not review the tests, but given the high coverage, these must be great! |
src/conversion.jl
Outdated
| # Are all these methods below really useful? | ||
| # The conversion to matrices is not something that belongs to ordinary use of LinearMaps. | ||
| # It's rather for debugging purposes, where efficiency is not the primary concern. | ||
| # Also, I don't think they are really that much more efficient |
There was a problem hiding this comment.
Generally, the Matrix conversion is used in the Kronecker code in the vec trick, which is why I started introducing these methods (at least with the intention to write code that is faster than the columnwise mat-vec-muls):
X there is promoted to a MatrixMap, that's why we have all those MatrixMap conversion methods here.
|
Ok, I thought I already left a message after previous commit, but clearly not. So I think now all comments have been removed (thanks @dkarrasch for checking carefully). The only remaining issue, which I did not address, is how to deal with |
|
I chose to remove |
|
Thank you so much, Jutho! Now let's discuss the FillMap constructor over at #113, and we're done with v3.0. 😂 |
Part 1, more to go (tomorrow or the days after that).