-
Notifications
You must be signed in to change notification settings - Fork 20
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
Exclude neighbours of the ref_index in DISE algorithm #223
base: main
Are you sure you want to change the base?
Conversation
@FanwangM and @marco-2023, the tests currently fail. If you agree with this change, I will update the tests. Just to let you know the OptiSim algorithm adds |
It looks good to me. Thanks for fixing this. |
# construct KDTree for quick nearest-neighbor lookup | ||
kdtree = scipy.spatial.KDTree(X) | ||
|
||
# construct bitarray to track selected samples (1 means exclude) | ||
bv = bitarray.bitarray(list(np.zeros(len(X), dtype=int))) | ||
bv[self.ref_index] = 1 |
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.
In the new code self.ref_index is also returned among the selected elements. Is this intended?
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.
note to myself: it is consistent with OptiSim
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.
The new function excludes points close to self.ref_index
from being selected, but self.ref_index
itself will be selected (this is consistent with the OptiSim algorithm). It looks good to me.
In DISE algorthim, the
ref_index
was not included as a selected sample, and its neighbors were not excluded. I believe this was not a desired feature, so this PR fixed that. Feel Free to let me know what you think.