add mergeID #3027
add mergeID #3027
Conversation
| H = setdiff(idx2, idx1) | ||
| idx_new = vcat(idx1, H) |
There was a problem hiding this comment.
I am not sure this is correct. It seems non-symmetric. Does the test still work in the other order?
There was a problem hiding this comment.
I think it's symmetric up to permutation. That is the merged zonotopes will have the same parameters, but these will not necessarily ordered by index (i.e. the index vector is not ordered). If one also sorted the index vector, then it would be symmetric I think
There was a problem hiding this comment.
at least looking at the algorithm description on page 35 of Niklas' thesis, the algorithm doesn't seem to be symmetric (because it doesn't fix the order)
There was a problem hiding this comment.
So when you do mergeID(PZ2, PZ1) in the test case, you get the IDs [2, 4, 1, 3]. Is this acceptable or should the indices be sorted?
There was a problem hiding this comment.
based on today's conversation, should we normalize the order?
There was a problem hiding this comment.
if in functions that require the same idvector (quadratic map, exact sum) you so something like
indexvector(PZ1) == indexvector(PZ2) || (PZ1, PZ2 = mergeID(PZ1, PZ2)I think even without normalizing the order it would be correct, because calling mergeID would not change PZ1, PZ2 as setdiff(idx2, idx1) would be empty on the other hand, I don't see harm in normalizing the order either
There was a problem hiding this comment.
I would not be surprised if some operations assume the same order after mergeID. So I would sort.
lucaferranti
force-pushed
the
lf-mergeID
branch
from
ff53d77 to
87ef0a6
Compare
|
What is the status of this PR? |
|
Closing because we probably do not need this. |
this is probably a brute force implementation of the pseudocode in niklas thesis (page 35), not sure if one can squeeze some more performance... I doubt that would be the bottleneck in any application anyway
Another question, should we have also an in-place version?