Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
add mergeID #3027
add mergeID #3027
Changes from all commits
87ef0a6
1d4ae3b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on today's conversation, should we normalize the order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if in functions that require the same idvector (quadratic map, exact sum) you so something like
I think even without normalizing the order it would be correct, because calling
mergeID
would not changePZ1, PZ2
assetdiff(idx2, idx1)
would be empty on the other hand, I don't see harm in normalizing the order eitherThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not be surprised if some operations assume the same order after
mergeID
. So I would sort.