different pivoting for hmatrices#5
Conversation
JoshuaTetzner
left a comment
JoshuaTetzner
left a comment
There was a problem hiding this comment.
The code linting tells you that you do try to call (::AdaptiveCrossApproximation.RandomSampling)(::Vector{Int64}, ::Vector{Int64}) but you do not have it implemented.
| npivots, U, V = cur_aca( | ||
| K, rowBuffer, colBuffer, maxrank; rowidcs=irange, colidcs=jrange | ||
| ) | ||
| aca = aca(K, Vector(irange), Vector(jrange)) |
There was a problem hiding this comment.
Cant we directly use the irange and jrange?
There was a problem hiding this comment.
The functions that existed in geometrical pivot strategies and are used here expect Vector{Int}, because of that and also since Vector can support index sets like 1, 4, 8, ... that are not consecutive and might be desired sometime in the fuiure I wrote everything based on Vectors
There was a problem hiding this comment.
I think it should not make any difference, since we only need the length which is defined for both. Accessing a Vector with both types is possible as well. Perhaps there is an abstract type containing both, do not define it at all or use a Union{} of Vector and Range.
There was a problem hiding this comment.
AbstractArray contains both, use AbstractArray or union?
There was a problem hiding this comment.
I just saw they already have AbstractArray when they return next index(e. g. in maxvalue.jl line 16), I'm looking now will union cause a problem or not
There was a problem hiding this comment.
I do not want to convert irange and jrange into a vector. Remove this first if it is still working with the abstractarray type leave it this way, if not change it to a Union of abstract array and Unit range
There was a problem hiding this comment.
used AbstractArray{Int}
I resolved this issue and the I failed the ambiguity test between the two functions at lines 33 and 40 of aca.jl. Their 'where' clauses were respectively: because of this I rewrote this part and wrote a single function to handle different pivoting cases for random sampling convergence. And for the sake of uniformity did the same for combined convergence too. hope that is fine. |
|
I think you look into the Rang vector stuff again. I left a comment there. |
|
locally on my laptop it passes the tests, including formatting. I have no idea |
* CI.yml aktualisieren * different pivoting for hmatrices (#5) * Small fix * Documentation Fix * Update README.md --------- Co-authored-by: Ali <88579506+alitaghdiri89@users.noreply.github.com>
2 participants
No description provided.